feat: modernize sdk/qrcode build pipeline #872
Conversation
|
Warning Rate limit exceeded@transphorm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRepackages sdk/qrcode for dual ESM/CJS outputs with explicit subpath exports, generates dist package metadata and CJS shims post-build, splits tsup into separate ESM/CJS builds, adds linting/tooling scripts, introduces WebSocket-driven SelfQRcode changes, and updates CI to verify new artifacts and exports. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / CI
participant TSUP as tsup (build)
participant TSC as tsc (declarations)
participant PB as postBuild.mjs
participant Dist as dist/
participant Consumer as App / Node
Dev->>TSUP: yarn build -> run tsup for esm + cjs configs
TSUP-->>Dist: emit files -> dist/esm/* and dist/cjs/*
Dev->>TSC: yarn build:types (tsc --emitDeclarationOnly)
TSC-->>Dist: emit .d.ts into dist/esm
Dev->>PB: yarn postbuild -> run postBuild.mjs
PB->>Dist: write dist/package.json, esm/package.json, cjs/package.json
PB->>Dist: create shims in dist/* (index.js + index.d.ts)
Consumer->>Dist: import/require subpaths (ESM or CJS)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
sdk/qrcode/package.json (1)
70-83: tsup should be a devDependency only (currently duplicated).tsup appears in both dependencies and devDependencies. It’s build-time only; remove it from dependencies to avoid bloating consumers.
"dependencies": { "@selfxyz/common": "workspace:^", "js-sha1": "^0.7.0", "js-sha256": "^0.11.0", "js-sha512": "^0.9.0", "lottie-react": "^2.4.0", "node-forge": "^1.3.1", "poseidon-lite": "^0.3.0", "qrcode.react": "^4.1.0", "react-spinners": "^0.14.1", "socket.io-client": "^4.8.1", - "tsup": "^8.5.0", "uuid": "^10.0.0" },
🧹 Nitpick comments (3)
sdk/qrcode/scripts/shimConfigs.js (1)
2-6: Nit: Drop unused name field or use it.name duplicates shimPath and is unused by postBuild. Remove it to reduce noise or put it to use (e.g., logging or default-export behavior flags).
Also applies to: 8-11, 12-16, 17-21, 22-26
sdk/qrcode/scripts/postBuild.mjs (1)
21-33: Clarify dist/package.json purpose and avoid confusion with root exports.dist/package.json exports only ESM paths; root package.json already provides full import/require/types mappings. Node will use root "exports" and block deep "@selfxyz/qrcode/dist/*" imports. The dist-level exports mainly help bundlers that bypass "exports". Consider a brief README note to explain intent and supported import paths.
Ensure your consumers don’t rely on deep "@selfxyz/qrcode/dist/*" requires, as Node 16+ with "exports" will reject them.
sdk/qrcode/package.json (1)
56-69: Script consistency: types should not re-run the full build."types": "yarn build" re-invokes the full pipeline. Make it call build:types directly.
"scripts": { - "build": "tsup && yarn build:types && yarn postbuild", + "build": "tsup && yarn build:types && yarn postbuild", "build:types": "tsc -p tsconfig.json --emitDeclarationOnly", "postbuild": "node ./scripts/postBuild.mjs", "build:watch": "tsup --watch", @@ - "types": "yarn build" + "types": "yarn build:types" },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
sdk/qrcode/package.json(1 hunks)sdk/qrcode/scripts/postBuild.mjs(1 hunks)sdk/qrcode/scripts/shimConfigs.js(1 hunks)sdk/qrcode/tsconfig.cjs.json(1 hunks)sdk/qrcode/tsup.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-01T17:11:39.232Z
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:
sdk/qrcode/scripts/shimConfigs.jssdk/qrcode/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). (2)
- GitHub Check: lint
- GitHub Check: type-check
🔇 Additional comments (4)
sdk/qrcode/scripts/shimConfigs.js (1)
1-27: Shim list verified — shimConfigs, tsup entries, and package.json exports are in syncChecked the three surfaces you called out and they match exactly (no breaking drift found):
- Files checked
- sdk/qrcode/scripts/shimConfigs.js — contains shims for:
- components/LED
- components/SelfQRcode
- utils/utils
- utils/styles
- utils/websocket
- sdk/qrcode/tsup.config.ts — entry map includes the same keys:
- components/LED, components/SelfQRcode, utils/utils, utils/styles, utils/websocket
- sdk/qrcode/package.json — exports include matching subpaths:
- ./components/LED, ./components/SelfQRcode, ./utils/utils, ./utils/styles, ./utils/websocket
- common/package.json exports present but unrelated to this shim set
Short recommendation (optional / low-effort):
- The original suggestion to derive the shim list from package.json exports is still a good one for maintainability. If you prefer to keep it static, add a small CI check (postbuild or lint step) that asserts parity between:
- shimConfigs, tsup entry keys, and package.json exports
This prevents future drift without forcing an immediate refactor.Conclusion: no action required to fix this PR — the three surfaces are aligned. Consider the automation/CI check suggestion to avoid future mismatches.
sdk/qrcode/package.json (2)
18-49: Exports look good and align with explicit subpaths.Explicit per-subpath import/require/types mappings will improve tree-shaking and DX. Nice alignment with prior learning about explicit exports aiding static analysis.
107-109: Engine range may be too strict for RN toolchains.node: ">=22 <23" can exclude common CI/dev environments (Node 18/20 LTS). Unless you require Node 22 features at runtime for this package, consider relaxing to an LTS range (e.g., ">=18") or move strictness to devEngines only.
sdk/qrcode/tsconfig.cjs.json (1)
1-9: CJS compiler options look correct.Extends base config, switches module to CommonJS, and disables declarations (handled by the ESM/tsc pass). LGTM.
transphorm
force-pushed
the
codex/implement-advanced-tsup-and-post-build-processing
branch
from
b9f5c1c to
1b5e80a
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
transphorm
changed the title
transphorm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (8)
app/scripts/mobile-deploy-confirm.cjs (2)
352-368: Guard nextBuild when currentBuild is non-numeric to avoid NaN in output.If Info.plist/pbxproj parse fails, currentBuild is "Unknown". parseInt('Unknown', 10) -> NaN, resulting in “Unknown → NaN”.
if (platform === PLATFORMS.IOS || platform === PLATFORMS.BOTH) { const currentBuild = versions.ios.build; - const nextBuild = versions.versionJson - ? versions.versionJson.ios.build + 1 - : parseInt(currentBuild, 10) + 1; + const currentBuildNum = Number.parseInt(String(currentBuild), 10); + const nextBuild = + versions.versionJson && Number.isFinite(versions.versionJson.ios.build) + ? versions.versionJson.ios.build + 1 + : Number.isFinite(currentBuildNum) + ? currentBuildNum + 1 + : 'Unknown';
370-388: Mirror the Android logic to avoid “Unknown → NaN”.if (platform === PLATFORMS.ANDROID || platform === PLATFORMS.BOTH) { const currentBuild = versions.android.versionCode; - const nextBuild = versions.versionJson - ? versions.versionJson.android.build + 1 - : parseInt(currentBuild, 10) + 1; + const currentBuildNum = Number.parseInt(String(currentBuild), 10); + const nextBuild = + versions.versionJson && Number.isFinite(versions.versionJson.android.build) + ? versions.versionJson.android.build + 1 + : Number.isFinite(currentBuildNum) + ? currentBuildNum + 1 + : 'Unknown';sdk/qrcode/utils/websocket.ts (3)
46-53: Do not log full SelfApp payload (PII leakage).Logging the entire selfApp can leak user identifiers and session data into logs. Log intent only or redact fields.
- console.log( - '[WebSocket] Mobile device connected. Emitting self_app event with payload:', - selfApp - ); + console.log('[WebSocket] Mobile device connected. Emitting self_app event.');
107-118: Guard the event handler call to prevent unhandled errors.Wrap handler execution in try/catch (and await if it returns a Promise). This ensures errors from onError/onSuccess don’t become unhandled rejections.
- socket.on('mobile_status', (data) => { + socket.on('mobile_status', async (data) => { console.log('[WebSocket] Raw mobile_status event received:', data); - handleWebSocketMessage( - socket, - sessionId, - selfApp, - type, - setProofStep, - onSuccess, - onError - )(data); + try { + await handleWebSocketMessage( + socket, + sessionId, + selfApp, + type, + setProofStep, + onSuccess, + onError + )(data); + } catch (err) { + console.error('[WebSocket] mobile_status handler threw:', err); + } });
24-30: Make WebSocket transports and path configurableHard-coding
transports: ['websocket']andpath: '/'can break connectivity (no polling fallback, corporate proxies) and diverges from Socket.IO’s default'/socket.io'. Let’s allow callers to override these settings and fall back to polling:• File: sdk/qrcode/utils/websocket.ts (lines 24–30)
• UpdatenewSocketsignature to accept an options object
• Defaulttransportsto['websocket', 'polling']andpathto'/socket.io'Suggested diff:
- const newSocket = (websocketUrl: string, sessionId: string) => { + const newSocket = ( + websocketUrl: string, + sessionId: string, + opts?: Partial<import('socket.io-client').ManagerOptions & import('socket.io-client').SocketOptions>, + ) => { const fullUrl = `${websocketUrl}/websocket`; console.log(`[WebSocket] Creating new socket. URL: ${fullUrl}, sessionId: ${sessionId}`); return io(fullUrl, { - path: '/', - query: { sessionId, clientType: 'web' }, - transports: ['websocket'], + path: opts?.path ?? '/socket.io', + query: { sessionId, clientType: 'web' }, + transports: opts?.transports ?? ['websocket', 'polling'], }); };Also, please verify that your server is listening on the chosen
path(default'/socket.io') or document the custom path in your deployment.sdk/qrcode/components/SelfQRcode.tsx (2)
98-106: Bug: Use Lottie, not Lottie.default
You’re importing a default component from lottie-react, but rendering Lottie.default. This will break at runtime. Render the default directly.Apply this diff:
- <Lottie.default + <Lottie animationData={X_ANIMATION} style={{ width: 200, height: 200 }} onComplete={() => { setProofStep(QRcodeSteps.WAITING_FOR_MOBILE); }} loop={false} />- <Lottie.default + <Lottie animationData={CHECK_ANIMATION} style={{ width: 200, height: 200 }} onComplete={() => { setProofStep(QRcodeSteps.WAITING_FOR_MOBILE); }} loop={false} />Also applies to: 109-117
56-71: Avoid stale onError in websocket effect dependencies
onError is passed into initWebSocket but missing from the effect deps. If the handler changes, the socket won’t be recreated and you’ll retain a stale callback.Apply this diff:
- }, [sessionId, type, websocketUrl, onSuccess, selfApp]); + }, [sessionId, type, websocketUrl, onSuccess, onError, selfApp]);Also applies to: 79-79
sdk/qrcode/package.json (1)
85-86: Don’t ship tsup to consumers; keep it in devDependencies only
tsup is listed in both dependencies and devDependencies. Tools should not be runtime deps of a library.Apply this diff:
"dependencies": { "@selfxyz/common": "workspace:^", "js-sha1": "^0.7.0", "js-sha256": "^0.11.0", "js-sha512": "^0.9.0", "lottie-react": "^2.4.0", "node-forge": "^1.3.1", "poseidon-lite": "^0.3.0", "qrcode.react": "^4.1.0", "react-spinners": "^0.14.1", "socket.io-client": "^4.8.1", - "tsup": "^8.5.0", "uuid": "^10.0.0" },(tsup is already present in devDependencies.)
Also applies to: 112-112
🧹 Nitpick comments (17)
app/tests/src/providers/passportDataProvider.test.tsx (1)
183-183: Explicit radix is good; consider stabilizing timer-driven test.
- parseInt(..., 10) is correct. Given RN Text children can be number or string, Number(...) is slightly safer.
- This test relies on setInterval + waitFor; consider jest fake timers to eliminate flakiness.
Example tweaks:
-const initialCount = parseInt(updateCount.props.children, 10); +const initialCount = Number(updateCount.props.children);Use fake timers to make this deterministic (outside this hunk):
// before render jest.useFakeTimers(); // after render act(() => { jest.advanceTimersByTime(400); }); // assert without waitFor expect(Number(getByTestId('update-count').props.children)).toBeGreaterThan(initialCount);Also applies to: 188-191
app/scripts/tests/tree-shaking.test.cjs (1)
27-27: Switch to octal literal LGTM; address lint false-positive and consider access().
- 0o111 is clearer than parseInt('111', 8).
- Static analysis flagged "&" usage; this is intentional for mode bits. Silence the rule locally.
Apply:
- const isExecutable = (stats.mode & 0o111) !== 0; + // eslint-disable-next-line no-bitwise + const isExecutable = (stats.mode & 0o111) !== 0;Optionally, prefer a POSIX check using fs.access when available:
// If desired, try access() first, fallback to mode bits try { fs.accessSync(scriptPath, fs.constants.X_OK); assert(true); } catch { // eslint-disable-next-line no-bitwise const isExecutable = (stats.mode & 0o111) !== 0; assert(isExecutable, `${script} should be executable`); }Note: On Windows, execute bits aren’t meaningful; if this test runs cross‑platform, guard or skip on win32.
app/scripts/bundle-analyze-ci.cjs (1)
28-29: Param rename is fine; add a defensive check for unknown platforms.BUNDLE_THRESHOLDS_MB is keyed by ios/android, but a guard helps future-proof.
function checkBundleSize(bundleSize, targetPlatform) { - const thresholdMB = BUNDLE_THRESHOLDS_MB[targetPlatform]; + const thresholdMB = BUNDLE_THRESHOLDS_MB[targetPlatform]; + if (typeof thresholdMB !== 'number') { + throw new Error(`Unknown platform "${targetPlatform}" in thresholds map`); + }app/scripts/mobile-deploy-confirm.cjs (1)
392-405: Avoid comparing NaN for build mismatch; bail out when actualBuild is not numeric.- const actualBuild = parseInt(versions.ios.build, 10); - if (jsonBuild !== actualBuild) { + const actualBuild = Number.parseInt(String(versions.ios.build), 10); + if (Number.isFinite(actualBuild) && jsonBuild !== actualBuild) { console.log( `\n${CONSOLE_SYMBOLS.WARNING} iOS build mismatch: version.json has ${jsonBuild}, but Xcode has ${actualBuild}`, ); }- const actualBuild = parseInt(versions.android.versionCode, 10); - if (jsonBuild !== actualBuild) { + const actualBuild = Number.parseInt(String(versions.android.versionCode), 10); + if (Number.isFinite(actualBuild) && jsonBuild !== actualBuild) { console.log( `\n${CONSOLE_SYMBOLS.WARNING} Android build mismatch: version.json has ${jsonBuild}, but gradle has ${actualBuild}`, ); }sdk/qrcode/.size-limit.json (1)
3-24: CJS path is fine for bundle budgets, but it disables tree‑shaking checks; keep an ESM target too. The 80 s time budget is very loose.
- size-limit’s per-import tests rely on ESM for treeshaking. Targeting dist/cjs/index.cjs makes “import: X” measurements less meaningful.
- Consider dual entries: one for size/time on CJS, another for treeshaking on ESM subpaths.
- Tighten thresholds (use kB budgets for size and ms for time) to catch regressions earlier.
Example additions:
{ "path": "./dist/esm/index.js", "name": "ESM: SelfQRcode import (tree-shaken)", "import": "SelfQRcode", "limit": "8 kB" }, { "path": "./dist/esm/index.js", "name": "ESM: countries import (tree-shaken)", "import": "countries", "limit": "2 kB" }If CI environments are noisy for time-based budgets, favor kB size budgets as primary and keep time budgets narrower (e.g., 1500 ms) to be useful.
Please confirm dist/esm is produced by tsup and exported via package.json so size-limit can resolve it in CI.
sdk/qrcode/utils/websocket.ts (1)
13-15: Avoid module-level console logs in a library.This logs on import, creating noise in consumer apps. Gate via a debug flag or remove.
common/tsup.config.ts (2)
89-90: outExtension on ESM config is redundant.This mapping affects only CJS format. Keeping it is harmless but may confuse future readers. Either remove or add a comment explaining symmetry with the CJS config.
10-86: DRY the duplicated entry maps to avoid drift.The ESM and CJS configs duplicate a very large entry map. Hoist a shared entries const and reuse it in both configs.
Use a shared entries object (outside the changed ranges):
// At top of the file const entries = { index: 'index.ts', // constants 'src/constants/index': 'src/constants/index.ts', 'src/constants/constants': 'src/constants/constants.ts', 'src/constants/countries': 'src/constants/countries.ts', 'src/constants/vkey': 'src/constants/vkey.ts', 'src/constants/skiPem': 'src/constants/skiPem.ts', 'src/constants/mockCertificates': 'src/constants/mockCertificates.ts', 'src/constants/sampleDataHashes': 'src/constants/sampleDataHashes.ts', // utils (grouped + granular) 'src/utils/index': 'src/utils/index.ts', 'src/utils/hash': 'src/utils/hash.ts', 'src/utils/bytes': 'src/utils/bytes.ts', 'src/utils/trees': 'src/utils/trees.ts', 'src/utils/scope': 'src/utils/scope.ts', 'src/utils/appType': 'src/utils/appType.ts', 'src/utils/date': 'src/utils/date.ts', 'src/utils/arrays': 'src/utils/arrays.ts', 'src/utils/types': 'src/utils/types.ts', 'src/utils/passports/index': 'src/utils/passports/index.ts', 'src/utils/passports/format': 'src/utils/passports/format.ts', 'src/utils/passports/mock': 'src/utils/passports/mock.ts', 'src/utils/passports/dg1': 'src/utils/passports/dg1.ts', 'src/utils/passports/genMockPassportData': 'src/utils/passports/genMockPassportData.ts', 'src/utils/passports/genMockIdDoc': 'src/utils/passports/genMockIdDoc.ts', 'src/utils/passports/passport_parsing/parseDscCertificateData': 'src/utils/passports/passport_parsing/parseDscCertificateData.ts', // certificate parsing 'src/utils/certificate_parsing/index': 'src/utils/certificate_parsing/index.ts', 'src/utils/certificate_parsing/elliptic': 'src/utils/certificate_parsing/elliptic.ts', 'src/utils/certificate_parsing/curves': 'src/utils/certificate_parsing/curves.ts', 'src/utils/certificate_parsing/oids': 'src/utils/certificate_parsing/oids.ts', 'src/utils/certificate_parsing/parseCertificateSimple': 'src/utils/certificate_parsing/parseCertificateSimple.ts', 'src/utils/certificate_parsing/parseSimple': 'src/utils/certificate_parsing/parseSimple.ts', 'src/utils/certificate_parsing/parseNode': 'src/utils/certificate_parsing/parseNode.ts', 'src/utils/certificate_parsing/ellipticInit': 'src/utils/certificate_parsing/ellipticInit.ts', 'src/utils/certificate_parsing/curveUtils': 'src/utils/certificate_parsing/curveUtils.ts', 'src/utils/certificate_parsing/oidUtils': 'src/utils/certificate_parsing/oidUtils.ts', 'src/utils/certificate_parsing/certUtils': 'src/utils/certificate_parsing/certUtils.ts', // circuits 'src/utils/circuits/index': 'src/utils/circuits/index.ts', 'src/utils/circuits/circuitsName': 'src/utils/circuits/circuitsName.ts', 'src/utils/circuits/formatOutputs': 'src/utils/circuits/formatOutputs.ts', 'src/utils/circuits/formatInputs': 'src/utils/circuits/formatInputs.ts', 'src/utils/circuits/uuid': 'src/utils/circuits/uuid.ts', 'src/utils/circuits/dscInputs': 'src/utils/circuits/dscInputs.ts', 'src/utils/circuits/registerInputs': 'src/utils/circuits/registerInputs.ts', 'src/utils/circuits/discloseInputs': 'src/utils/circuits/discloseInputs.ts', 'src/utils/circuits/generateInputs': 'src/utils/circuits/generateInputs.ts', 'src/utils/circuits/ofacInputs': 'src/utils/circuits/ofacInputs.ts', // contracts 'src/utils/contracts/index': 'src/utils/contracts/index.ts', 'src/utils/contracts/forbiddenCountries': 'src/utils/contracts/forbiddenCountries.ts', 'src/utils/csca': 'src/utils/csca.ts', // passport functions 'src/utils/passports/core': 'src/utils/passports/core.ts', 'src/utils/passports/commitment': 'src/utils/passports/commitment.ts', 'src/utils/passports/signature': 'src/utils/passports/signature.ts', 'src/utils/passports/parsing': 'src/utils/passports/parsing.ts', 'src/utils/passports/mockGeneration': 'src/utils/passports/mockGeneration.ts', 'src/utils/passports/mockDsc': 'src/utils/passports/mockDsc.ts', 'src/utils/passports/passport': 'src/utils/passports/passport.ts', // types 'src/types/index': 'src/types/index.ts', 'src/types/passport': 'src/types/passport.ts', 'src/types/app': 'src/types/app.ts', 'src/types/certificates': 'src/types/certificates.ts', 'src/types/circuits': 'src/types/circuits.ts', };Then replace both blocks with:
- entry: { /* many entries */ }, + entry: entries,Also applies to: 122-197
sdk/qrcode/.eslintrc.cjs (1)
1-36: Solid ESLint baseline; add minor DX tweaks.Looks good. Consider:
- Add simple-import-sort/exports to enforce export ordering.
- Add overrides for config/test files to relax stricter rules where needed (e.g., dev scripts).
- If you enable any type-aware rules later, set parserOptions.project to include tsconfig.cjs.json as well.
.github/workflows/qrcode-sdk-ci.yml (2)
183-196: Also validate ESM import path.You only test CJS import. Add an ESM import smoke test to catch export-map or extension issues.
- name: Test package import run: | echo "Testing package import..." node -e " try { const { SelfQRcode, SelfQRcodeWrapper, countries } = require('./sdk/qrcode/dist/cjs/index.cjs'); console.log('✅ Package import successful'); console.log('Exported components:', Object.keys({ SelfQRcode, SelfQRcodeWrapper, countries })); } catch (error) { console.error('❌ Package import failed:', error.message); process.exit(1); } " + - name: Test ESM import + run: | + echo "Testing ESM import..." + node --input-type=module -e " + try { + const mod = await import('./sdk/qrcode/dist/esm/index.js'); + console.log('✅ ESM import successful'); + console.log('Exported keys:', Object.keys(mod)); + } catch (error) { + console.error('❌ ESM import failed:', error.message); + process.exit(1); + } + "
88-99: SDK cache key may go stale; include config/type artifacts.Keying only on package.json can serve stale builds. Include tsup/tsconfig/scripts and yarn.lock to better invalidate.
- key: ${{ runner.os }}-sdk-build-${{ env.GH_CACHE_VERSION }}-${{ env.GH_SDK_CACHE_VERSION }}-${{ hashFiles('sdk/qrcode/package.json', 'common/package.json') }} + key: ${{ runner.os }}-sdk-build-${{ env.GH_CACHE_VERSION }}-${{ env.GH_SDK_CACHE_VERSION }}-${{ hashFiles('yarn.lock', 'sdk/qrcode/package.json', 'common/package.json', 'sdk/qrcode/tsup.config.ts', 'sdk/qrcode/tsconfig.json', 'sdk/qrcode/tsconfig.cjs.json', 'sdk/qrcode/scripts/**', 'common/tsup.config.ts', 'common/tsconfig*.json') }}Also applies to: 158-169
sdk/qrcode/components/SelfQRcode.tsx (4)
58-58: Gate console logs behind a debug condition
These logs will show up in consumers’ consoles. Consider gating behind NODE_ENV or a debug prop.Example:
- console.log('[QRCode] Initializing new WebSocket connection'); + if (process.env.NODE_ENV !== 'production') { + // eslint-disable-next-line no-console + console.log('[QRCode] Initializing new WebSocket connection'); + }- console.log('[QRCode] Cleaning up WebSocket connection'); + if (process.env.NODE_ENV !== 'production') { + // eslint-disable-next-line no-console + console.log('[QRCode] Cleaning up WebSocket connection'); + }Also applies to: 73-73
16-25: Prop ‘children’ is declared but never rendered
SelfQRcodeProps includes children, but they aren’t used. Either remove the prop or render it (e.g., as a footer/overlay) to prevent confusion.Example:
- If unused: remove children from the props interface.
- If intended: render {children} inside the container.
Also applies to: 140-141
120-132: Safer QR value construction for URL robustness
When type === 'websocket', concatenating query params risks double-? or missing encoding if REDIRECT_URL already has params. Prefer URL/URLSearchParams.Apply this diff to make the websocket path robust:
- ? `${REDIRECT_URL}?sessionId=${sessionId}` + ? (() => { + try { + const url = new URL(REDIRECT_URL); + url.searchParams.set('sessionId', sessionId); + return url.toString(); + } catch { + const sep = REDIRECT_URL.includes('?') ? '&' : '?'; + return `${REDIRECT_URL}${sep}sessionId=${encodeURIComponent(sessionId)}`; + } + })()
50-50: Type clarity for socketRef (nit)
ReturnType works, but being explicit makes intent clearer if initWebSocket’s signature evolves.For example:
type WebSocketCleanup = () => void; const socketRef = useRef<WebSocketCleanup | null>(null);sdk/qrcode/package.json (2)
121-121: Engines range is very restrictive
Node ">=22 <23" will block many consumers (CI/CD often pins to LTS 18/20). Unless you rely on Node 22-only features at runtime, consider relaxing to ">=18" or ">=20". If Node 22 is required (e.g., JSON import assertions in published JS), call it out in the README and ensure your ESM output is compatible with bundlers.Proposed:
- "node": ">=22 <23" + "node": ">=18"
10-16: Good sideEffects marking for non-pure modules
Calling out websocket and animation assets as having side effects helps prevent unsafe tree-shaking. Consider if SelfQRcode.cjs/js is still necessary to mark given component modules are typically side-effect free; if the side effects are purely due to JSON animation imports, you might scope sideEffects to animations only.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
.github/workflows/qrcode-sdk-ci.yml(1 hunks)app/scripts/bundle-analyze-ci.cjs(1 hunks)app/scripts/mobile-deploy-confirm.cjs(4 hunks)app/scripts/tests/tree-shaking.test.cjs(1 hunks)app/src/providers/authProvider.web.tsx(1 hunks)app/src/screens/prove/ProofRequestStatusScreen.tsx(1 hunks)app/tests/src/providers/passportDataProvider.test.tsx(1 hunks)common/index.ts(1 hunks)common/tsup.config.ts(2 hunks)sdk/qrcode/.eslintrc.cjs(1 hunks)sdk/qrcode/.prettierrc(1 hunks)sdk/qrcode/.size-limit.json(1 hunks)sdk/qrcode/README.md(1 hunks)sdk/qrcode/components/LED.tsx(2 hunks)sdk/qrcode/components/SelfQRcode.tsx(3 hunks)sdk/qrcode/index.ts(1 hunks)sdk/qrcode/package.json(2 hunks)sdk/qrcode/scripts/postBuild.mjs(1 hunks)sdk/qrcode/tsup.config.ts(1 hunks)sdk/qrcode/utils/styles.ts(1 hunks)sdk/qrcode/utils/websocket.ts(5 hunks)
✅ Files skipped from review due to trivial changes (3)
- sdk/qrcode/utils/styles.ts
- sdk/qrcode/README.md
- sdk/qrcode/components/LED.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/qrcode/scripts/postBuild.mjs
🧰 Additional context used
📓 Path-based instructions (2)
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/screens/prove/ProofRequestStatusScreen.tsxapp/src/providers/authProvider.web.tsx
**/*.{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/src/providers/passportDataProvider.test.tsx
🧠 Learnings (2)
📚 Learning: 2025-08-01T17:11:39.232Z
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:
sdk/qrcode/package.json
📚 Learning: 2025-07-28T19:18:48.270Z
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:
sdk/qrcode/package.json
🧬 Code Graph Analysis (1)
sdk/qrcode/tsup.config.ts (2)
app/scripts/test-tree-shaking.cjs (1)
entries(278-278)sdk/qrcode/scripts/postBuild.mjs (1)
__dirname(7-7)
🪛 GitHub Check: lint
app/scripts/tests/tree-shaking.test.cjs
[warning] 27-27:
Unexpected use of '&'
⏰ 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
- GitHub Check: run_circuit_tests
🔇 Additional comments (12)
app/src/providers/authProvider.web.tsx (1)
194-198: Good use of unknown in catch clause.Renaming to err and narrowing with instanceof Error is idiomatic and type-safe.
sdk/qrcode/.prettierrc (1)
2-16: Prettier config looks sane; confirm monorepo consistency.
- arrowParens: "always" and trailingComma: "es5" will touch many files; ensure this doesn’t fight the root config.
- endOfLine: "lf" is preferred in CI; fine for cross‑platform.
Please verify:
- There’s no conflicting Prettier config at repo root.
- eslint/prettier plugins in sdk/qrcode/.eslintrc.cjs align with these rules to avoid churn.
sdk/qrcode/utils/websocket.ts (1)
51-53: Behavior change: gating self_app emission only for 'websocket'. Verify deeplink flow.Previously both transports might have emitted; now deeplink skips emission. Confirm that deeplink flows don’t require self_app emission or update the deeplink flow to emit appropriately.
common/tsup.config.ts (2)
95-96: Platform neutral for a library is the right call.Neutral avoids environment-specific shims and plays nicely across web/RN/Node.
102-118: Externalizing Node built-ins: confirm consumption patterns.Marking fs/path/child_process as external is correct for Node-only entry points but will error if a browser/RN consumer imports those entry subpaths. Given you expose many granular entry points, this is fine so long as only Node contexts import Node-targeted entries. Please confirm that your package.json exports or documentation clarify which entries are Node-only.
Also applies to: 213-229
sdk/qrcode/tsup.config.ts (3)
19-21: outExtension mapping is correct and aligns with .cjs require targets.Good job ensuring CJS artifacts use .cjs to match exports for type: module packages.
Also applies to: 40-42
21-23: Types are generated elsewhere; ensure the build script covers it.Since dts: false, verify your workspace build runs the types step and that dist/esm contains .d.ts referenced by exports.
Also applies to: 41-44
4-12: Nice DRYing of entry maps and dual-config split.Shared entries and per-format dist/ dirs are clean and maintainable.
Also applies to: 37-41
common/index.ts (1)
38-38: Expose REDIRECT_URL looks good; confirm env-parity (staging/prod)
Exporting REDIRECT_URL is aligned with the new QR flow usage. Please confirm there’s an equivalent staging-safe variant or that REDIRECT_URL switches based on Mode/ENV (like API_URL and WS_DB_RELAYER), so websocket and deeplink flows don’t accidentally point to prod in non-prod environments.sdk/qrcode/index.ts (1)
1-12: API surface looks clean and tree-shakeable
Good separation of type-only exports and values, and explicit re-exports. This should aid bundlers and aligns with the new subpath exports.sdk/qrcode/package.json (2)
20-48: Confirm .d.ts emission paths match export map
You reference types under dist/esm (root and subpaths). Ensure tsc --emitDeclarationOnly outputs to dist/esm and includes all components/utils. If not, consumers will get missing type declarations.Action items:
- Verify tsconfig.json sets "outDir": "dist/esm" (or you pass it via CLI).
- Confirm after yarn build, the following exist:
- dist/esm/index.d.ts
- dist/esm/components/SelfQRcode.d.ts
- dist/esm/components/LED.d.ts
- dist/esm/utils/{websocket,styles,utils}.d.ts
If any are missing, adjust "build:types" or tsconfig.Also applies to: 50-53
56-73: Build pipeline scripts look consistent
tsup -> tsc declarations -> postbuild shims/versioning is a solid flow, and aligns with the dual-distribution strategy.
| const url = new URL(selfApp.deeplinkCallback); | ||
| if (url) { | ||
| setCountdown(5); | ||
| setCountdownStarted(true); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate protocol and avoid redundant if (url); store validated URL for use on redirect.
- new URL(...) already guarantees a truthy object; the if (url) guard is redundant.
- For security, explicitly allow only known-safe protocols and store the validated value to use later with Linking.openURL, preventing TOCTOU drift if selfApp.deeplinkCallback changes.
Apply:
- const url = new URL(selfApp.deeplinkCallback);
- if (url) {
- setCountdown(5);
- setCountdownStarted(true);
- }
+ const url = new URL(selfApp.deeplinkCallback);
+ const allowedProtocols = new Set(['https:', 'http:', 'self:']); // adjust as needed
+ if (allowedProtocols.has(url.protocol)) {
+ setCountdown(5);
+ setCountdownStarted(true);
+ setValidatedDeeplink(url.toString());
+ } else {
+ console.warn('Unsupported deep link protocol:', url.protocol);
+ }Also update outside this hunk:
- Add state near other state hooks:
const [validatedDeeplink, setValidatedDeeplink] = useState<string | null>(null);- Use it on redirect:
if (validatedDeeplink) {
Linking.openURL(validatedDeeplink).catch(err => { /* ... */ });
}- And for UI display:
deeplinkCallback={validatedDeeplink?.replace(/^https?:\/\//, '')}🤖 Prompt for AI Agents
In app/src/screens/prove/ProofRequestStatusScreen.tsx around lines 89-93, the if
(url) check is redundant and the deeplinkCallback should be validated and stored
to avoid TOCTOU and unsafe protocols; replace the redundant guard by validating
new URL(selfApp.deeplinkCallback) for allowed protocols (e.g. http, https, your
app-scheme) and on success call setValidatedDeeplink with the sanitized string
and start the countdown (setCountdown/setCountdownStarted); add a new state near
the other hooks: validatedDeeplink (string | null) as suggested, use
validatedDeeplink for Linking.openURL in the redirect (with proper catch) and
for UI display (strip protocol when rendering) so the code always uses the
pre-validated URL rather than reading selfApp.deeplinkCallback later.
| if (websocketUrl.includes('localhost') || websocketUrl.includes('127.0.0.1')) { | ||
| throw new Error('localhost websocket URLs are not allowed'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen URL validation (cover ::1 and ensure correct host parsing).
String includes checks are brittle and can block/allow unintended hosts. Parse the URL and compare hostname against disallowed loopback hosts.
-const validateWebSocketUrl = (websocketUrl: string) => {
- if (websocketUrl.includes('localhost') || websocketUrl.includes('127.0.0.1')) {
- throw new Error('localhost websocket URLs are not allowed');
- }
-};
+const validateWebSocketUrl = (websocketUrl: string) => {
+ try {
+ const { hostname } = new URL(websocketUrl);
+ const disallowed = new Set(['localhost', '127.0.0.1', '::1']);
+ if (disallowed.has(hostname)) {
+ throw new Error('localhost/loopback websocket URLs are not allowed');
+ }
+ } catch {
+ throw new Error('Invalid websocket URL');
+ }
+};🤖 Prompt for AI Agents
In sdk/qrcode/utils/websocket.ts around lines 17 to 20, the current validation
uses string includes which is brittle; replace it by parsing websocketUrl with
the URL constructor (catching invalid-URL errors) and compare url.hostname
exactly against disallowed loopback hosts ('localhost', '127.0.0.1', and '::1');
if the hostname matches any of those throw the same error, otherwise allow;
ensure this handles ports and IPv6 bracketed forms correctly by relying on
URL.hostname.
| ) => | ||
| async (data: any) => { | ||
| console.log( | ||
| '[WebSocket] Received mobile status:', | ||
| data.status, | ||
| 'for session:', | ||
| sessionId, | ||
| ); | ||
| async (data: { status: string; error_code?: string; reason?: string }) => { | ||
| console.log('[WebSocket] Received mobile status:', data.status, 'for session:', sessionId); | ||
| switch (data.status) { | ||
| case 'mobile_connected': |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary async and align error payload type.
The returned handler doesn’t await anything. Remove async to avoid unhandled Promise nuances and widen onError payload to accept status when forwarded.
- onSuccess: () => void,
- onError: (data: { error_code?: string; reason?: string }) => void
+ onSuccess: () => void,
+ onError: (data: { status?: string; error_code?: string; reason?: string }) => void
) =>
- async (data: { status: string; error_code?: string; reason?: string }) => {
+ (data: { status: string; error_code?: string; reason?: string }) => {📝 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.
| onError: (data: { error_code?: string; reason?: string }) => void | |
| ) => | |
| async (data: any) => { | |
| console.log( | |
| '[WebSocket] Received mobile status:', | |
| data.status, | |
| 'for session:', | |
| sessionId, | |
| ); | |
| async (data: { status: string; error_code?: string; reason?: string }) => { | |
| console.log('[WebSocket] Received mobile status:', data.status, 'for session:', sessionId); | |
| switch (data.status) { | |
| onSuccess: () => void, | |
| onError: (data: { status?: string; error_code?: string; reason?: string }) => void | |
| ) => | |
| (data: { status: string; error_code?: string; reason?: string }) => { | |
| console.log('[WebSocket] Received mobile status:', data.status, 'for session:', sessionId); | |
| switch (data.status) { |
🤖 Prompt for AI Agents
In sdk/qrcode/utils/websocket.ts around lines 40 to 44, the returned handler is
marked async though it doesn't await anything and the onError type only allows
error_code/reason; remove the unnecessary async from the returned function to
avoid unhandled Promise behavior, and broaden the onError payload type to
include status (e.g., { status: string; error_code?: string; reason?: string })
so when you forward errors you can pass status along; update the function
signature and any callers to match the new onError type.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
.github/workflows/qrcode-sdk-ci.yml (2)
31-35: Good fix: Node is now pinned via setup-node in all jobs.This addresses the earlier risk of jobs running with the runner’s default Node. Thanks for adding actions/setup-node@v4.
Also applies to: 80-85, 138-143, 212-217
183-188: Robust type-def check looks good.Switching from bare ls to a find-based assertion prevents glob-fail pitfalls and yields clearer errors on missing .d.ts.
🧹 Nitpick comments (5)
.github/workflows/qrcode-sdk-ci.yml (4)
102-116: Order steps: restore artifacts before dependency install (or skip restore entirely).If you keep restoring node_modules (not recommended), it should happen before Install Dependencies. With the previous change to stop restoring node_modules, this becomes moot. If you still need to reorder, place “Restore build artifacts” above “Install Dependencies”.
200-205: Add an ESM import smoke test to complement the CJS require.Guard against ESM regressions by importing the ESM entry under Node’s ESM loader.
- name: Test package import run: | echo "Testing package import..." node -e " try { const { SelfQRcode, SelfQRcodeWrapper, countries } = require('./sdk/qrcode/dist/cjs/index.cjs'); console.log('✅ Package import successful'); console.log('Exported components:', Object.keys({ SelfQRcode, SelfQRcodeWrapper, countries })); } catch (error) { console.error('❌ Package import failed:', error.message); process.exit(1); } " + + - name: Test ESM package import + run: | + echo "Testing ESM package import..." + node --input-type=module -e " + try { + const mod = await import('./sdk/qrcode/dist/esm/index.js'); + console.log('✅ ESM import successful:', Object.keys(mod)); + } catch (error) { + console.error('❌ ESM import failed:', error.message); + process.exit(1); + } + "Also applies to: 252-265
11-23: Harden workflow permissions (principle of least privilege).Set minimal permissions at the workflow root.
on: pull_request: @@ push: branches: [main, develop] paths: - "sdk/qrcode/**" - "common/**" + +permissions: + contents: read
31-36: Remove double-caching of Yarn (setup-node cache + actions/cache).You currently cache Yarn via setup-node’s cache: 'yarn' and via actions/cache of .yarn/cache. Choose one to avoid redundant downloads. Given you already manage .yarn/cache explicitly (good for Yarn Berry), consider dropping cache: 'yarn' from setup-node.
- name: Use Node.js ${{ env.NODE_VERSION }} uses: actions/setup-node@v4 with: node-version: ${{ env.NODE_VERSION }} - cache: 'yarn'Repeat in all jobs.
Also applies to: 89-101, 147-159, 221-233
sdk/qrcode/tsup.config.ts (1)
31-39: Tighten externals regex to avoid accidental matches (e.g., react-native)./^react/ is broad and can externalize unintended packages. Anchor fully-qualified externals and include jsx-dev-runtime.
- external: [ - /^react/, - /^react-dom/, - /^react\/jsx-runtime$/, - /^lottie-react/, - /^qrcode.react/, - /^socket.io-client/, - /^node-forge/, - ], + external: [ + /^react$/, + /^react-dom$/, + /^react\/jsx-runtime$/, + /^react\/jsx-dev-runtime$/, + /^lottie-react$/, + /^qrcode\.react$/, + /^socket\.io-client$/, + /^node-forge$/, + ],Apply in both build configs.
Also applies to: 53-61
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/qrcode-sdk-ci.yml(1 hunks)sdk/qrcode/tsup.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-14T09:03:08.292Z
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/workflows/qrcode-sdk-ci.yml
🧬 Code Graph Analysis (1)
sdk/qrcode/tsup.config.ts (2)
common/scripts/testExports.js (1)
__filename(12-12)sdk/qrcode/scripts/postBuild.mjs (1)
__dirname(7-7)
⏰ 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: run_circuit_tests
- GitHub Check: e2e-ios
- GitHub Check: analyze-android
🔇 Additional comments (2)
sdk/qrcode/tsup.config.ts (2)
1-16: Solid ESM-safe config scaffolding.ESM-safe __dirname via fileURLToPath and shared entries map are on point. This keeps both builds in sync and avoids loader pitfalls.
18-40: Dual ESM/CJS config with .cjs outExtension, neutral platform, and sourcemaps: LGTM.Matches package.json exports strategy and CI verification. outExtension ensures require() lines up with .cjs. Platform ‘neutral’ keeps RN/web consumers happy.
Also applies to: 41-63
transphorm
deleted the
codex/implement-advanced-tsup-and-post-build-processing
branch
Summary
Testing
yarn lint(fails: 317 warnings)yarn buildyarn workspace @selfxyz/contracts build(fails: HH8 config error)yarn typesyarn workspace @selfxyz/common testyarn workspace @selfxyz/circuits test(fails: unsupported signature algorithm)yarn workspace @selfxyz/mobile-app testyarn workspace @selfxyz/mobile-sdk-alpha testhttps://chatgpt.com/codex/tasks/task_b_6899336eb664832db3261c4d0cb4f00d
Summary by CodeRabbit
New Features
Chores
Bug Fixes / Maintenance