Fix web app screen loading #812
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughA new utility function, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (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: 0
♻️ Duplicate comments (1)
app/src/navigation/prove.ts (1)
7-19: Preserve prop generics as in previous comment
All of theselazyScreen()usages will benefit once the helper exposes the generic type parameter.
🧹 Nitpick comments (4)
app/src/navigation/lazyScreen.tsx (1)
6-15: Consider preserving the wrapped component’s identity and allowing a custom fallback
LazyScreenWrapperloses the original component’sdisplayNameand is recreated for every invocation oflazyScreen, making stack traces less helpful. A tiny tweak gives you better DX at zero runtime cost and lets callers override the fallback when needed.export function lazyScreen<T extends object>( importer: () => Promise<{ default: React.ComponentType<T> }>, -): React.ComponentType<T> { + fallback: React.ReactNode = <View />, +): React.ComponentType<T> { const LazyComp = React.lazy(importer) as unknown as React.ComponentType<T>; - const LazyScreenWrapper = (props: T) => ( - <Suspense fallback={<View />}> + const LazyScreenWrapper: React.FC<T> = (props) => ( + <Suspense fallback={fallback}> <LazyComp {...(props as T)} /> </Suspense> ); + LazyScreenWrapper.displayName = + `Lazy(${LazyComp.displayName ?? LazyComp.name ?? 'Screen'})`; return LazyScreenWrapper as React.ComponentType<T>; }Benefits: clearer error traces, optional spinners/place-holders per screen, and no change to the call-sites if you rely on the default.
app/src/navigation/recovery.ts (1)
5-24: Heavy repetition – extract a tiny factory to keep this list tidyEight near-identical
lazyScreen(() => import(...))declarations inflate the file. A one-liner helper keeps intent obvious and reduces churn when you add or rename screens.const lazy = (path: string) => lazyScreen(() => import(`../screens/recovery/${path}`)); const AccountRecoveryChoiceScreen = lazy('AccountRecoveryChoiceScreen'); const AccountRecoveryScreen = lazy('AccountRecoveryScreen'); const AccountVerifiedSuccessScreen = lazy('AccountVerifiedSuccessScreen'); const PassportDataNotFound = lazy('PassportDataNotFoundScreen'); const RecoverWithPhraseScreen = lazy('RecoverWithPhraseScreen'); const SaveRecoveryPhraseScreen = lazy('SaveRecoveryPhraseScreen');Same pattern applies to the other navigation modules touched in this PR.
app/src/navigation/dev.ts (1)
5-20: Looks good, tiny style nitTrailing comma after the closing parenthesis of
MockDataScreenDeepLinkkeeps prettier/ESLint happy and avoids noisy diffs when appending props later.-const MockDataScreenDeepLink = lazyScreen( - () => import('../screens/dev/MockDataScreenDeepLink'), -); +const MockDataScreenDeepLink = lazyScreen( + () => import('../screens/dev/MockDataScreenDeepLink'), +);app/src/navigation/settings.ts (1)
7-21: ExposelazyScreen’s generic so screen-prop types aren’t lost
lazyScreen()almost certainly returnsReact.ComponentType<any>, which silently discards the actual prop contract of each screen. This undermines type-safety for navigation params and can hide run-time bugs.A minimal tweak inside
lazyScreen.tsxkeeps the generic intact:-export function lazyScreen(loader: () => Promise<{ default: React.ComponentType<any> }>) { - const Component = React.lazy(loader); - return (props: any) => ( +export function lazyScreen<P = {}>(loader: () => Promise<{ default: React.ComponentType<P> }>) { + const Component = React.lazy(loader); + return (props: P) => ( <Suspense fallback={<View />}> <Component {...props} /> </Suspense> ); }Benefits:
• IntelliSense & compile-time checks for screen props remain intact.
• No migration required at call-sites.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/navigation/aesop.ts(1 hunks)app/src/navigation/dev.ts(1 hunks)app/src/navigation/lazyScreen.tsx(1 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)
🧰 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/navigation/passport.tsapp/src/navigation/aesop.tsapp/src/navigation/recovery.tsapp/src/navigation/dev.tsapp/src/navigation/lazyScreen.tsxapp/src/navigation/settings.tsapp/src/navigation/prove.ts
⏰ 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: analyze-android
- GitHub Check: analyze-ios
- GitHub Check: build
🔇 Additional comments (5)
app/src/navigation/aesop.ts (1)
7-11: Switch tolazyScreenlooks goodImport replacement is clean, and
PassportOnboardingScreenis now wrapped consistently with the new helper. No other changes required here.app/src/navigation/passport.ts (1)
5-26: LGTM – consistent lazy wrapper adoptionAll passport screens now use the shared helper, matching the rest of the navigation layer.
app/src/navigation/settings.ts (1)
5-6: Centralising lazy-loading throughlazyScreenis on point
Import looks correct and removes repetitive<Suspense>boilerplate from every navigation file.app/src/navigation/prove.ts (2)
5-6: Good swap tolazyScreen
Keeps navigation files consistent with the new helper.
17-18: ✅ Import path verified – no update needed
The fileapp/src/screens/prove/ViewFinderScreen.tsxexists and matches the lazy import, soQRCodeViewFinderScreenwill load correctly.
|
doesn't work closing in favor of: #814 |
transphorm
deleted the
codex/implement-lazyscreen-and-update-navigation-files
branch
Summary
lazyScreenhelper component to wrap dynamic imports in<Suspense>lazyScreeninstead ofReact.lazyacross navigation modulesTesting
yarn lintyarn buildyarn workspace @selfxyz/contracts build(fails: Invalid account for hardhat)yarn typesyarn workspace @selfxyz/common testyarn workspace @selfxyz/circuits test(fails: Unsupported signature algorithm)yarn workspace @selfxyz/mobile-app testyarn workspace @selfxyz/mobile-app web(no "Objects are not valid as a React child" error)https://chatgpt.com/codex/tasks/task_b_688a6860bf88832da3f866cdd8ed7854
Summary by CodeRabbit
New Features
Refactor