SEL-508: Implement code splitting and lazy loading #795
Conversation
WalkthroughThis update introduces dynamic imports and lazy loading for multiple screen components, adds new utility modules for dynamic resource loading, and reorganizes import paths for improved modularity. It also enhances bundler and build configurations, updates workflow automation, and includes memory optimizations for Android builds. No exported APIs or public interfaces were changed. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant SafeAreaProvider
participant ErrorBoundary
participant Providers
participant Suspense
participant Navigation
App->>SafeAreaProvider: Render
SafeAreaProvider->>ErrorBoundary: Render
ErrorBoundary->>Providers: Render
Providers->>Suspense: Render
Suspense->>Navigation: Render (with fallback)
Navigation-->>Suspense: Loads screens (some via React.lazy)
Suspense-->>Providers: Show fallback if loading
sequenceDiagram
participant Navigation
participant React.lazy
participant SuspenseFallback
Navigation->>React.lazy: Request screen component (e.g., PassportOnboardingScreen)
React.lazy-->>Navigation: Loads component asynchronously
alt Loading
Navigation->>SuspenseFallback: Show "Loading..." UI
else Loaded
Navigation->>Screen: Render screen
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 9
🔭 Outside diff range comments (2)
app/src/screens/dev/MockDataScreenDeepLink.tsx (1)
51-51: Replace explicit any with proper typing.The type assertion
as anycircumvents TypeScript's type safety and is flagged by ESLint. Consider using a more specific type or type guard.-nationality: storeState.deepLinkNationality as any, +nationality: storeState.deepLinkNationality as keyof typeof countryCodes,app/src/navigation/index.tsx (1)
69-69: Remove console.log statement to fix linting violation.The ESLint rule
no-consoleis violated by this console.log statement. Consider using a proper logging utility or removing debug logging.- console.log(`Screen View: ${currentRoute.name}`);
🧹 Nitpick comments (4)
app/src/utils/proving/provingUtils.ts (1)
3-6: Consider lazy-loadingnode-forgeto cut initial bundle weight.
node-forgeis ~700 KB min-gzip; pulling it into the main chunk undercuts the code-splitting effort. A tiny refactor pushes it to an async path:-import { WS_DB_RELAYER, WS_DB_RELAYER_STAGING } from '@selfxyz/common'; -import type { EndpointType } from '@selfxyz/common'; -import { initElliptic } from '@selfxyz/common'; -import forge from 'node-forge'; +import { WS_DB_RELAYER, WS_DB_RELAYER_STAGING } from '@selfxyz/common'; +import type { EndpointType } from '@selfxyz/common'; +import { initElliptic } from '@selfxyz/common'; + +// Lazy-load to keep the critical path lean +let forge: typeof import('node-forge') | null = null; +async function getForge() { + if (!forge) forge = await import('node-forge'); + return forge; +}Then in
encryptAES256GCM:-export function encryptAES256GCM( +export async function encryptAES256GCM( plaintext: string, key: forge.util.ByteStringBuffer, ) { - const iv = forge.random.getBytesSync(12); + const f = await getForge(); + const iv = f.random.getBytesSync(12); + const cipher = f.cipher.createCipher('AES-GCM', key);This defers the heft until encryption is actually invoked.
app/src/assets/animations/loader.ts (1)
3-4: Return the JSON object directly for smoother call-sites.Consumers typically want the parsed JSON, not the module wrapper. A tiny tweak avoids the extra
.then(m => m.default)everywhere:-export const loadPassportAnimation = () => import('./passport_verify.json'); -export const loadMiscAnimation = () => import('./loading/misc.json'); +export const loadPassportAnimation = () => + import('./passport_verify.json').then(m => m.default); +export const loadMiscAnimation = () => + import('./loading/misc.json').then(m => m.default);app/src/providers/passportDataProvider.tsx (1)
47-47: Consider consolidating imports to avoid duplicationThe separate import statement for
DocumentCategoryandPassportDatamay be causing the import duplication warnings flagged by ESLint. Consider keeping these imports grouped with the other imports from@selfxyz/commonfor better organization.The pipeline shows import/no-duplicates warnings. Please consolidate the imports from
@selfxyz/common:-import { - brutforceSignatureAlgorithmDsc, - parseCertificateSimple, - PublicKeyDetailsECDSA, - PublicKeyDetailsRSA, -} from '@selfxyz/common'; -import { DocumentCategory, PassportData } from '@selfxyz/common'; +import { + brutforceSignatureAlgorithmDsc, + parseCertificateSimple, + PublicKeyDetailsECDSA, + PublicKeyDetailsRSA, + DocumentCategory, + PassportData, +} from '@selfxyz/common';app/vite.config.ts (1)
101-110: Consider chunk size optimization for screen bundles.While the screen-specific chunking is well-organized, consider monitoring chunk sizes to ensure optimal loading performance. Large screen chunks might need further subdivision.
Add chunk size monitoring to your build process:
// Consider adding to rollupOptions output: { manualChunks: { // existing chunks... }, // Add chunk size warnings chunkFileNames: (chunkInfo) => { const facadeModuleId = chunkInfo.facadeModuleId ? chunkInfo.facadeModuleId.split('/').pop() : 'chunk'; return `[name]-[hash].js`; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
app/App.tsx(2 hunks)app/src/assets/animations/loader.ts(1 hunks)app/src/navigation/aesop.ts(1 hunks)app/src/navigation/dev.ts(1 hunks)app/src/navigation/index.tsx(2 hunks)app/src/navigation/passport.ts(1 hunks)app/src/navigation/prove.ts(1 hunks)app/src/navigation/recovery.ts(1 hunks)app/src/navigation/settings.ts(1 hunks)app/src/providers/passportDataProvider.tsx(1 hunks)app/src/screens/dev/MockDataScreen.tsx(1 hunks)app/src/screens/dev/MockDataScreenDeepLink.tsx(1 hunks)app/src/screens/passport/PassportNFCScanScreen.tsx(1 hunks)app/src/screens/prove/ProveScreen.tsx(1 hunks)app/src/stores/proof-types.ts(1 hunks)app/src/stores/selfAppStore.tsx(1 hunks)app/src/utils/cryptoLoader.ts(1 hunks)app/src/utils/proving/provingInputs.ts(1 hunks)app/src/utils/proving/provingMachine.ts(1 hunks)app/src/utils/proving/provingUtils.ts(1 hunks)app/src/utils/proving/validateDocument.ts(1 hunks)app/tsconfig.json(1 hunks)app/vite.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/dev/MockDataScreen.tsxapp/src/screens/passport/PassportNFCScanScreen.tsxapp/src/screens/prove/ProveScreen.tsxapp/src/utils/proving/provingInputs.tsapp/src/navigation/aesop.tsapp/src/utils/proving/provingUtils.tsapp/src/utils/proving/validateDocument.tsapp/src/navigation/recovery.tsapp/src/navigation/passport.tsapp/src/providers/passportDataProvider.tsxapp/src/utils/proving/provingMachine.tsapp/src/navigation/dev.tsapp/src/screens/dev/MockDataScreenDeepLink.tsxapp/src/utils/cryptoLoader.tsapp/src/stores/selfAppStore.tsxapp/src/navigation/prove.tsapp/src/navigation/settings.tsapp/src/assets/animations/loader.tsapp/src/navigation/index.tsxapp/src/stores/proof-types.ts
🧠 Learnings (2)
📓 Common learnings
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.
app/src/screens/passport/PassportNFCScanScreen.tsx (1)
Learnt from: transphorm
PR: #636
File: app/src/components/native/QRCodeScanner.tsx:135-142
Timestamp: 2025-07-16T02:20:44.173Z
Learning: In app/src/components/native/QRCodeScanner.tsx, the Android camera dimension multipliers (screenWidth * 3 and screenHeight * 2) are intentionally set to these values and should not be changed. These multipliers are correct and any visual issues with black areas in the camera preview are caused by other factors, not the dimension calculations.
🪛 GitHub Actions: General Self CI
app/src/screens/dev/MockDataScreen.tsx
[warning] 10-250: ESLint warnings: Multiple 'Unexpected console statement' (no-console) and 'imported multiple times' (import/no-duplicates) issues detected.
app/src/screens/passport/PassportNFCScanScreen.tsx
[warning] 8-341: ESLint warnings: Multiple 'Unexpected console statement' (no-console), 'Unexpected any' (@typescript-eslint/no-explicit-any), and 'imported multiple times' (import/no-duplicates) issues detected.
app/src/screens/prove/ProveScreen.tsx
[error] 5-5: ESLint error: Unable to resolve path to module '@selfxyz/common/utils/scope' (import/no-unresolved).
app/src/providers/passportDataProvider.tsx
[warning] 46-486: ESLint warnings: Multiple 'Unexpected console statement' (no-console) and 'imported multiple times' (import/no-duplicates) issues detected.
app/src/screens/dev/MockDataScreenDeepLink.tsx
[warning] 4-51: ESLint warnings: 'imported multiple times' (import/no-duplicates) and 'Unexpected any' (@typescript-eslint/no-explicit-any) issues detected.
app/src/navigation/index.tsx
[warning] 69-69: ESLint warning: 'Unexpected console statement' (no-console) issue detected.
🪛 GitHub Check: lint
app/src/screens/prove/ProveScreen.tsx
[failure] 5-5:
Unable to resolve path to module '@selfxyz/common/utils/scope'
⏰ 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: build
🔇 Additional comments (20)
app/src/stores/selfAppStore.tsx (1)
3-4: Import split looks good.Moving
SelfAppbehindimport typecleanly strips it from the runtime bundle while keeping the value import forWS_DB_RELAYER. No issues spotted.app/src/utils/proving/provingInputs.ts (1)
17-23: Type-only import is a tidy win.Converting
DocumentCategory,PassportData, etc. to a pure type import avoids bundling unused runtime objects. 👍app/src/stores/proof-types.ts (1)
3-3: Pure type import is correct.
EndpointTypeandUserIdTypeare referenced only in the type system, so this change prevents an unnecessary runtime dependency.app/tsconfig.json (1)
5-5: LGTM! Module configuration supports code-splittingThe explicit
"module": "esnext"setting properly enables dynamic import syntax and supports the lazy loading implementation throughout the app. This aligns well with the PR's code-splitting objectives.app/src/utils/proving/provingMachine.ts (1)
7-7: LGTM! Proper use of type-only importsConverting the imports to
import typeis excellent practice when these types are only used for type annotations. This optimization prevents unnecessary runtime code inclusion and aligns with TypeScript best practices for tree-shaking.app/src/screens/prove/ProveScreen.tsx (1)
4-4: Excellent TypeScript optimization!Using type-only imports for
SelfAppDisclosureConfighelps reduce bundle size by preventing runtime inclusion of type definitions. This aligns well with the code-splitting objectives of this PR.app/src/navigation/recovery.ts (1)
4-23: Excellent code-splitting implementation!The conversion to lazy-loaded imports for all recovery screens is well-executed and aligns perfectly with the PR's code-splitting objectives. This will significantly improve initial load times by deferring the loading of recovery screens until they're actually needed.
The consistent pattern across all screen components maintains good code organization and readability.
app/src/navigation/passport.ts (1)
4-26: Consistent and effective code-splitting pattern!The lazy loading implementation for passport screens mirrors the excellent pattern used in the recovery navigation. This consistent approach across navigation modules will provide significant performance benefits by reducing the initial bundle size and enabling on-demand loading of passport functionality.
app/src/navigation/aesop.ts (1)
4-11: Consistent application of lazy loading pattern!The conversion of
PassportOnboardingScreento lazy loading maintains consistency with the code-splitting approach applied across all navigation modules. Even with just one screen, this consistent pattern contributes to the overall bundle optimization strategy.app/App.tsx (3)
6-6: Excellent addition of React imports for lazy loading.Properly imports
lazyandSuspensefrom React to support the code splitting implementation.
16-20: Well-implemented lazy loading pattern.The PassportProvider is correctly converted to a lazy-loaded component using React.lazy with proper named export extraction. This will help reduce the initial bundle size.
34-42: Proper Suspense boundary implementation.The Suspense wrapper with LoadingScreen fallback provides excellent UX during the lazy loading of PassportProvider. The provider nesting structure is maintained correctly.
app/src/navigation/dev.ts (1)
4-18: Systematic lazy loading implementation for dev screens.All development screen components are properly converted to lazy imports using React.lazy. This enables effective code splitting for development screens, which is particularly beneficial since these screens can be excluded or loaded on-demand in production builds.
app/src/navigation/prove.ts (1)
4-18: Consistent lazy loading pattern for prove screens.All prove-related screen components are systematically converted to lazy imports, maintaining consistency with the code splitting strategy across navigation modules. This will improve initial load performance for the core proof functionality.
app/src/navigation/settings.ts (1)
4-18: Effective lazy loading for settings screens.Settings screen components are properly converted to lazy imports, which is particularly beneficial since settings screens are accessed less frequently than core functionality. This implementation supports the overall code splitting strategy effectively.
app/src/utils/cryptoLoader.ts (1)
4-8: Excellent concurrent loading pattern for performance.The use of
Promise.allfor concurrent dynamic imports is well implemented and will significantly improve loading performance compared to sequential imports.app/src/navigation/index.tsx (2)
87-89: Excellent Suspense implementation for lazy-loaded navigation.The Suspense wrapper around the Navigation component is properly implemented and will handle lazy-loaded screen components gracefully. This aligns well with the code-splitting objectives.
9-9: Proper React import update for Suspense.The import statement correctly includes Suspense, which is essential for the lazy loading implementation.
app/vite.config.ts (2)
66-113: Well-designed chunking strategy that aligns perfectly with code-splitting goals.The manual chunk configuration is excellently structured with logical groupings:
- Vendor libraries separated by functionality (React, UI, crypto, device, analytics)
- Screen-specific chunks align with lazy loading implementation
- Crypto chunk matches the cryptoLoader.ts dynamic imports
This will significantly improve initial load times and enable efficient code splitting.
77-85: Crypto chunk aligns well with cryptoLoader utility.The vendor-crypto chunk includes the same libraries (
elliptic,node-forge,ethers) that are dynamically imported incryptoLoader.ts, ensuring consistent bundling strategy.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/general-checks.yml (1)
12-13:yarn buildat repo-root may be fragile & slow — tighten the scope or guard it.Running
yarn buildfrom the monorepo root triggers every package’s build script.
• Cycles that don’t need a build (e.g. docs packages) will still execute, adding minutes to CI.
• Packages with native add-ons or platform-specific steps can fail on Ubuntu, blocking the lint job.Consider one of these tweaks:
- - name: Build Dependencies - run: yarn build + - name: Build Dependencies + # Only build packages that expose a build script; ignore silent packages. + run: yarn workspaces foreach --topological-dev --if-present run buildor, if only the mobile app truly needs compiling:
run: yarn workspace @selfxyz/mobile-app buildEither approach keeps the safety net while trimming CI time and unexpected breakage.
.github/workflows/app.yml (1)
25-27: Duplicated build work — align withbuild:deps(test job) to avoid doing the same work twice.The lint job now compiles the app with
yarn build, but the test job later callsyarn build:deps. Unless these scripts perform distinct tasks, we’re burning CI minutes twice and maintaining two commands.If they are equivalent, unify them for consistency:
- - name: Build Dependencies - run: yarn build + - name: Build Dependencies + run: yarn build:deps # already used in the test jobIf they’re intentionally different, drop a quick comment explaining the distinction so future maintainers don’t accidentally “simplify” it away.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/app.yml(1 hunks).github/workflows/general-checks.yml(1 hunks)app/src/screens/prove/ProofRequestStatusScreen.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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.tsx
🧬 Code Graph Analysis (1)
app/src/screens/prove/ProofRequestStatusScreen.tsx (1)
app/src/utils/haptic/index.ts (1)
buttonTap(19-19)
🔇 Additional comments (3)
app/src/screens/prove/ProofRequestStatusScreen.tsx (3)
5-5: LGTM: Import optimization aligns with code splitting goalsAdding
useCallbackanduseRefto the React import statement is a good practice that supports the performance optimizations made in this component. This change aligns well with the PR's code splitting and lazy loading objectives.
51-51: LGTM: Consistent hook usage patternChanging from
React.useRefto directuseRefusage is consistent with the other hooks in the component and follows modern React patterns. This maintains consistency with the updated import statement.
53-59: Excellent performance optimization with useCallbackConverting
onOkPressto a memoized callback prevents unnecessary re-renders of child components that receive this function as a prop. The dependency array[goHome, cleanSelfApp]is correctly specified, ensuring the callback updates when these dependencies change.
| selfApp, | ||
| countdownStarted, | ||
| countdown, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify effect dependency changes for correctness
The dependency changes need careful consideration:
-
selfApp?.deeplinkCallback→selfApp: This broadens the dependency scope, potentially causing more frequent effect executions when anyselfAppproperty changes, not justdeeplinkCallback. -
Adding
countdown: This creates a potential infinite loop risk since the effect can modifycountdownstate (line 94:setCountdown(5)), which would then retrigger the effect.
Please verify that these dependency changes don't cause performance issues or infinite loops. Consider whether selfApp?.deeplinkCallback was more appropriate as a dependency, or if countdown should be excluded from dependencies to prevent the effect from retriggering when it sets the countdown value.
🏁 Script executed:
#!/bin/bash
# Search for other useEffect patterns with countdown dependencies in the codebase
ast-grep --pattern 'useEffect($_, [$$$, countdown, $$$])'
# Search for similar patterns where an effect sets state that's also in its dependency array
rg -A 10 -B 5 "useEffect.*countdown.*setCountdown"Length of output: 7203
Narrow the first useEffect’s dependency array to avoid unintended re-runs
I verified that in ProofRequestStatusScreen.tsx there are exactly two useEffect hooks:
- The second one (lines 140–160) correctly depends on
[countdown, selfApp?.deeplinkCallback, onOkPress]for the timer/redirect logic. - The first one (lines 74–138, deps at 127–138) currently lists
which causes the entire effect (including notifications, status update, and countdown start) to re-run whenever any property on
currentState, isFocused, appName, sessionId, errorCode, reason, updateProofStatus, selfApp, countdownStarted, countdown,
selfAppchanges or whencountdownStarted/countdownupdate—leading to duplicate side-effects.
Recommendations:
- Replace
selfAppwith the specificselfApp?.deeplinkCallbackdependency. - Remove
countdownStartedandcountdownfrom this array (they belong only in the timer effect). - If you need to guard the countdown start, consider a separate
useEffector memoizing the callback.
Suggested diff:
}, [
- currentState,
- isFocused,
- appName,
- sessionId,
- errorCode,
- reason,
- updateProofStatus,
- selfApp,
- countdownStarted,
- countdown,
+ currentState,
+ isFocused,
+ appName,
+ sessionId,
+ errorCode,
+ reason,
+ updateProofStatus,
+ selfApp?.deeplinkCallback,
]);🤖 Prompt for AI Agents
In app/src/screens/prove/ProofRequestStatusScreen.tsx around lines 127 to 138,
the first useEffect hook's dependency array is too broad, including the entire
selfApp object and countdown-related variables, causing unintended re-runs and
duplicate side-effects. Narrow the dependencies by replacing selfApp with
selfApp?.deeplinkCallback, and remove countdownStarted and countdown from this
array. If countdown start logic needs guarding, move it to a separate useEffect
or memoize the callback to prevent unnecessary executions.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (4)
app/src/utils/proving/attest.ts (4)
113-114: Remove @ts-ignore and fix type safety issueUsing
@ts-ignoresuppresses TypeScript's type checking, which defeats the purpose of using TypeScript for type safety. This is particularly concerning in security-critical code handling attestation verification.- //@ts-ignore - if (!attestationDoc[field]) { + if (!(field in attestationDoc) || !attestationDoc[field as keyof AttestationDoc]) {Alternatively, consider creating a proper type guard function to validate the attestation document structure.
176-181: Security concern: PCR0 validation bypass in developmentWhile bypassing PCR0 validation in development mode may be convenient, this creates a significant security risk if the development flag is accidentally enabled in production or if the condition logic is compromised.
Consider implementing a more secure approach:
- Use environment-specific configuration files instead of
__DEV__- Add additional validation layers even in development mode
- Log security bypasses more prominently with timestamps
- if (!isPCR0Set && !__DEV__) { - throw new Error('Invalid image hash'); - } - if (__DEV__ && !isPCR0Set) { - console.warn('\x1b[31m%s\x1b[0m', '⚠️ WARNING: PCR0 CHECK SKIPPED ⚠️'); - } + if (!isPCR0Set) { + if (process.env.NODE_ENV === 'production') { + throw new Error('Invalid image hash'); + } else { + console.error('\x1b[31m%s\x1b[0m', `⚠️ SECURITY WARNING: PCR0 CHECK BYPASSED IN ${process.env.NODE_ENV} MODE - ${new Date().toISOString()} ⚠️`); + } + }
228-228: Potential null pointer dereferenceThe non-null assertion operator (!) is used on
base64.match()without proper validation, which could cause runtime errors if the regex fails to match.- base64.match(/.{1,64}/g)!.join('\n') + + (base64.match(/.{1,64}/g) || []).join('\n') +
100-202: Consider refactoring large function for better maintainabilityThe
verifyAttestationfunction handles multiple responsibilities (parsing, validation, certificate verification, cryptographic verification) and is over 100 lines long. This makes it harder to test, debug, and maintain.Consider breaking it down into smaller, focused functions:
export const verifyAttestation = async (attestation: Array<number>) => { const attestationDoc = await parseAndValidateAttestation(attestation); await validateAttestationStructure(attestationDoc); await verifyPCR0Mapping(attestation); await verifyCertificateChain(attestationDoc); await verifyCryptographicSignature(attestation, attestationDoc); return true; }; const parseAndValidateAttestation = async (attestation: Array<number>) => { // COSE_Sign1 parsing logic }; const validateAttestationStructure = async (attestationDoc: AttestationDoc) => { // Field validation logic };This approach improves:
- Testability (each function can be unit tested independently)
- Readability and maintainability
- Error isolation and debugging
- Code reusability
🧹 Nitpick comments (3)
app/src/screens/misc/LoadingScreen.tsx (1)
39-207: Consider refactoring for better separation of concerns.While the current implementation works, this component has significant responsibilities: data loading, state management, animation control, and UI rendering. Consider extracting some logic into custom hooks for better maintainability:
- Extract passport data loading logic into a custom hook
- Separate animation state management
- Consider using a reducer for complex state transitions
This would improve testability and make the component easier to understand.
app/src/screens/settings/PassportDataInfoScreen.tsx (1)
18-18: Address the TODO comment for data display requirements.The TODO comment indicates uncertainty about which metadata keys should be displayed. Consider clarifying the requirements to ensure the most relevant passport metadata is shown to users.
Would you like me to help analyze the current metadata keys and suggest which ones might be most valuable for user display?
app/src/utils/proving/attest.ts (1)
269-279: Move type definitions to top of file or separate moduleThe
AttestationDoctype definition is placed in the middle of the implementation code. For better code organization and TypeScript best practices, consider moving type definitions to the top of the file or a separate types module.+ // Move to top of file after imports + type AttestationDoc = { + module_id: string; + digest: string; + timestamp: number; + pcrs: { [key: number]: Buffer }; + certificate: Buffer; + cabundle: Array<Buffer>; + public_key: string | null; + user_data: string | null; + nonce: string | null; + }; + // ... rest of implementation - type AttestationDoc = { - module_id: string; - digest: string; - timestamp: number; - pcrs: { [key: number]: Buffer }; - certificate: Buffer; - cabundle: Array<Buffer>; - public_key: string | null; - user_data: string | null; - nonce: string | null; - };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/app.yml(1 hunks).github/workflows/general-checks.yml(2 hunks)app/src/components/Disclosures.tsx(1 hunks)app/src/providers/passportDataProvider.tsx(1 hunks)app/src/screens/dev/MockDataScreen.tsx(1 hunks)app/src/screens/dev/MockDataScreenDeepLink.tsx(1 hunks)app/src/screens/misc/LoadingScreen.tsx(1 hunks)app/src/screens/passport/PassportNFCScanScreen.tsx(1 hunks)app/src/screens/prove/ProveScreen.tsx(1 hunks)app/src/screens/settings/PassportDataInfoScreen.tsx(1 hunks)app/src/stores/proof-types.ts(1 hunks)app/src/stores/proofHistoryStore.ts(1 hunks)app/src/stores/protocolStore.ts(1 hunks)app/src/stores/selfAppStore.tsx(1 hunks)app/src/utils/nfcScanner.ts(1 hunks)app/src/utils/ofac.ts(1 hunks)app/src/utils/proving/attest.ts(1 hunks)app/src/utils/proving/provingInputs.ts(1 hunks)app/src/utils/proving/provingMachine.ts(1 hunks)app/src/utils/proving/provingUtils.ts(1 hunks)app/src/utils/proving/validateDocument.ts(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- app/src/stores/proofHistoryStore.ts
- app/src/utils/ofac.ts
- app/src/stores/protocolStore.ts
- app/src/stores/proof-types.ts
- app/src/screens/prove/ProveScreen.tsx
- app/src/stores/selfAppStore.tsx
- app/src/screens/dev/MockDataScreenDeepLink.tsx
- app/src/utils/proving/provingUtils.ts
- app/src/screens/passport/PassportNFCScanScreen.tsx
- app/src/components/Disclosures.tsx
- app/src/screens/dev/MockDataScreen.tsx
- app/src/providers/passportDataProvider.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/general-checks.yml
- .github/workflows/app.yml
- app/src/utils/proving/provingInputs.ts
- app/src/utils/proving/provingMachine.ts
- app/src/utils/proving/validateDocument.ts
🧰 Additional context used
📓 Path-based instructions (1)
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/nfcScanner.tsapp/src/utils/proving/attest.tsapp/src/screens/settings/PassportDataInfoScreen.tsxapp/src/screens/misc/LoadingScreen.tsx
🧠 Learnings (1)
app/src/screens/misc/LoadingScreen.tsx (1)
Learnt from: transphorm
PR: #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/.
🔇 Additional comments (4)
app/src/utils/nfcScanner.ts (1)
5-5: LGTM! Type-only import correctly implemented.The change from a direct import to a type-only import for
PassportDatais appropriate since it's only used for type annotations. This aligns with the PR's code splitting objectives and improves bundle optimization.app/src/screens/misc/LoadingScreen.tsx (1)
4-4: LGTM! Type-only import correctly implemented.The change to a type-only import for
PassportDatais appropriate since it's only used for type annotations. This supports the code splitting optimization goals.app/src/screens/settings/PassportDataInfoScreen.tsx (1)
4-4: LGTM! Type-only import with appropriate granular path.The change to a type-only import for
PassportMetadatais correct since it's only used for type annotations. The specific import path from the passport parsing submodule is appropriately granular and aligns with the code splitting optimization strategy.app/src/utils/proving/attest.ts (1)
4-7: LGTM: Import refactoring improves modularityThe change from importing directly from
@selfxyz/commonto the more specific@selfxyz/common/constants/constantsaligns with the broader refactoring effort to improve import hygiene and modularity across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/package.json (1)
27-29: Extract the Bundler + CocoaPods step into its own reusable scriptThe
"cd ios && bundle install && bundle exec pod install && cd .."sequence is now duplicated in bothinstall-appandinstall-app:setup. Duplicated shell fragments tend to drift over time (e.g., one gains a--repo-updateor--deploymentflag while the other does not), which can lead to subtle CI / local-build mismatches.A small helper script keeps the intent DRY and makes future tweaks a one-liner:
@@ - "install-app": "yarn install-app:setup && cd ios && bundle install && bundle exec pod install && cd .. && yarn clean:xcode-env-local", + "pod:install": "cd ios && bundle install --deployment && bundle exec pod install && cd ..", + "install-app": "yarn install-app:setup && yarn pod:install && yarn clean:xcode-env-local", @@ - "install-app:setup": "yarn install && yarn build:deps && cd ios && bundle install && bundle exec pod install && cd ..", + "install-app:setup": "yarn install && yarn build:deps && yarn pod:install",Benefits:
• Eliminates duplication and risk of divergence
• Adds--deploymenttobundle installfor reproducible gem versions (optional but recommended)
• Makes future flags (--repo-update, etc.) a single-point changeapp/tests/utils/proving/provingMachine.generatePayload.test.ts (2)
136-166: Test coverage could be enhanced with edge cases and error scenarios.While the current tests cover the happy path for all three circuit types, consider adding test cases for:
- Error handling when
getSolidityPackedUserContextDatafails- Invalid circuit types
- Missing or malformed store state
- Encryption failures in
encryptAES256GCMThe current assertions are good but could be more comprehensive. For example, verifying the complete payload structure rather than just selected properties.
Consider adding error scenario tests:
it('should handle hash function errors gracefully', async () => { const mockHashError = jest.fn().mockRejectedValue(new Error('Hash failed')); jest.mocked(require('@selfxyz/common/utils/hash').getSolidityPackedUserContextData) .mockImplementation(mockHashError); useProvingStore.setState({ circuitType: 'register' }); await expect(useProvingStore.getState()._generatePayload()) .rejects.toThrow('Hash failed'); });
23-23: Consider using more realistic mock data for better test fidelity.The fixed return value
'0x1234'forgetSolidityPackedUserContextDatais simple but may not catch issues related to data format or length validation. Consider using a more realistic hash-like value that better represents actual function output.- getSolidityPackedUserContextData: jest.fn(() => '0x1234'), + getSolidityPackedUserContextData: jest.fn(() => '0x1234567890abcdef1234567890abcdef12345678'),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
app/Gemfile(1 hunks)app/package.json(1 hunks)app/src/utils/proving/provingMachine.ts(1 hunks)app/tests/utils/proving/provingMachine.generatePayload.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/Gemfile
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/utils/proving/provingMachine.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/proving/provingMachine.generatePayload.test.ts
🧠 Learnings (1)
app/tests/utils/proving/provingMachine.generatePayload.test.ts (1)
Learnt from: transphorm
PR: #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/.
🔇 Additional comments (2)
app/tests/utils/proving/provingMachine.generatePayload.test.ts (2)
19-25: Well-targeted mock refinement improves test specificity.The change from mocking the entire
@selfxyz/commonmodule to specifically targeting@selfxyz/common/utils/hashis a solid improvement. This approach provides better isolation and aligns with the modularization efforts across the codebase.The preservation of actual implementation while overriding only
getSolidityPackedUserContextDatafollows testing best practices.
74-134: Comprehensive test setup enhances reliability.The beforeEach setup is thorough and properly initializes all required store states. This approach ensures consistent test conditions and reduces flakiness. The use of
jest.clearAllMocks()is particularly good for preventing test interdependencies.The mock data appears realistic and covers the necessary properties for the proving machine functionality.
| - uses: actions/checkout@v4 | ||
| - name: Install Dependencies | ||
| uses: ./.github/actions/yarn-install | ||
| - name: Build Dependencies |
There was a problem hiding this comment.
build dependencies so yarn lint does fail when looking up the nested file paths
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/metro.config.cjs (1)
48-70: Consider improvements to the custom resolver implementationWhile the resolver correctly handles TypeScript module resolution, consider these enhancements:
- The path check using string matching could be more robust
- Multiple resolution attempts might impact performance in large codebases
resolveRequest: (context, moduleName, platform) => { - // If this is a relative import ending in .js from within the common package - if (moduleName.endsWith('.js') && context.originModulePath.includes('/common/src/')) { + // Only process .js imports from the common package source + const isFromCommonSrc = context.originModulePath.includes(path.join('common', 'src')); + if (moduleName.endsWith('.js') && isFromCommonSrc) { const tsModuleName = moduleName.replace(/\.js$/, '.ts'); const tsxModuleName = moduleName.replace(/\.js$/, '.tsx'); // Try to resolve as .ts first, then .tsx try { return context.resolveRequest(context, tsModuleName, platform); - } catch { + } catch (tsError) { try { return context.resolveRequest(context, tsxModuleName, platform); - } catch { + } catch (tsxError) { // Fall back to default resolution return context.resolveRequest(context, moduleName, platform); } } } // Default resolution for everything else return context.resolveRequest(context, moduleName, platform); },This makes the path check more reliable and preserves error information for debugging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/App.tsx(2 hunks)app/metro.config.cjs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/App.tsx
🧰 Additional context used
🧠 Learnings (1)
app/metro.config.cjs (1)
Learnt from: transphorm
PR: #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/.
⏰ 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: build
🔇 Additional comments (3)
app/metro.config.cjs (3)
34-34: Good choice for enabling code splittingDisabling import/export transformation is the correct approach for supporting dynamic imports and code splitting. This preserves ES module syntax, which is essential for the lazy loading implementation mentioned in the PR objectives.
46-47: Appropriate resolver configuration for multi-platform supportThe
resolverMainFieldspriority order correctly prefers React Native implementations while maintaining web compatibility. Adding 'web' to platforms aligns with modern React Native apps that target multiple platforms.
15-15: Verify production bundling of @selfxyz/common alias
Before merging, please confirm that pointing Metro at the common package’ssrcfolder won’t break your release build—since the package’sexportsonly referencedist. In particular:• Ensure Metro’s transformer setup (Babel + TypeScript) is applied in production so all
.ts/.tsxfiles under@selfxyz/common/srcare correctly transpiled.
• Double-check thatdisableImportExportTransformdoesn’t prevent any module syntax Metro needs at runtime.
• Validate your custom resolver handles index files and deep imports (e.g.@selfxyz/common/utils/foo.ts) exactly as it would when resolving the compileddistoutputs.
• Run a production-mode bundle locally (or in CI) and smoke-test any code paths that depend on common—watch for missing polyfills or runtime errors caused by skipping post-build transformations (the post-build script only emitspackage.jsonin eachdistsubfolder).If you run into hiccups in CI or on-device, consider aliasing to
common/distin production or conditionally swapping the alias based onprocess.env.NODE_ENV.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/android/app/build.gradle (1)
52-79: Dynamic Hermes path concatenation may mis-handle Windows path separatorsString-concatenating
"/"when buildingdynamicPathrisks producing mixed separators on Windows (e.g.C:\foo/bar\baz.exe). Although the JVM usually tolerates this, Gradle’s exec task occasionally doesn’t, and it’s safer to rely onnew Fileto join path fragments:- def dynamicPath = "${hermesPath}/${platformDir}/${executableName}" + def dynamicPath = new File(new File(hermesPath, platformDir), executableName).absolutePathKeeps the logic portable without affecting *nix builds.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/android/app/build.gradle(3 hunks)app/android/gradle.properties(1 hunks)app/metro.config.cjs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/android/gradle.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- app/metro.config.cjs
🧰 Additional context used
📓 Path-based instructions (1)
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
| // Build optimizations | ||
| dexOptions { | ||
| javaMaxHeapSize "4g" | ||
| preDexLibraries false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
dexOptions block is deprecated in AGP 7+
javaMaxHeapSize is now honoured automatically by the D8 pipeline and preDexLibraries was removed. Newer AGP versions ignore (and will soon error on) this stanza. Consider:
// AGP 7.4+
android {
buildTypes.configureEach {
dexing {
jumboMode = true // if required
}
}
// Gradle JVM heap (when memory is the issue)
tasks.withType(JavaCompile).configureEach {
options.forkOptions.memoryMaximumSize = "4g"
}
}This future-proofs the build and avoids warning spam.
🤖 Prompt for AI Agents
In app/android/app/build.gradle around lines 114 to 118, the dexOptions block is
deprecated in Android Gradle Plugin 7 and above. Remove the entire dexOptions
block and instead configure dexing options inside the android buildTypes block,
such as enabling jumboMode if needed. Also, set the Java compile task's
forkOptions.memoryMaximumSize to "4g" to control heap size. This update aligns
with AGP 7+ requirements and prevents build warnings or errors.
- Regenerated Gemfile.lock with GITHUB_ACTIONS=true to match the conditional nokogiri exclusion in the Gemfile - Removes nokogiri and its dependencies (mini_portile2, racc) from lock file - Fixes pipeline error about deleted nokogiri dependency
| // Dynamic path that works across different platforms (macOS, Linux, Windows) | ||
| hermesCommand = { | ||
| def hermesPath = "../node_modules/react-native/sdks/hermesc" | ||
| def hermesPath = file("${rootProject.projectDir}/../../node_modules/react-native/sdks/hermesc").absolutePath |
There was a problem hiding this comment.
fix hermes path issues when building from the android directory
| "clean:android": "rm -rf android/app/build android/build", | ||
| "clean:build": "rm -rf ios/build android/app/build android/build", | ||
| "clean:ios": "rm -rf ios/Pods ios/Podfile.lock", | ||
| "clean:ios": "rm -rf ios/Pods ios/Podfile.lock && rm -rf Gemfile.lock", |
There was a problem hiding this comment.
fixes stale Gemfile.lock
| "format": "yarn nice", | ||
| "ia": "yarn install-app", | ||
| "install-app": "yarn install-app:setup && cd ios && bundle exec pod install && cd .. && yarn clean:xcode-env-local", | ||
| "install-app": "yarn install-app:setup && cd ios && bundle install && bundle exec pod install && cd .. && yarn clean:xcode-env-local", |
There was a problem hiding this comment.
we need to run bundle install AND bundle exec pod install
if not, we run into weird build pipeline errors:
https://github.com/selfxyz/self/actions/runs/16587650502/job/46916055932?pr=795
hackertron
deleted the
codex/implement-code-splitting-for-react-native-web-app
branch
hackertron
restored the
codex/implement-code-splitting-for-react-native-web-app
branch
Summary
vite.config.tsTesting
yarn lintyarn buildyarn workspace @selfxyz/contracts build(fails: Invalid account in config)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_68879ec34774832d8a74781f0833f1a7
Summary by CodeRabbit
New Features
Improvements
Chores
Style/Refactor