-
Notifications
You must be signed in to change notification settings - Fork 180
Chore: Fix web building and loading #814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis change removes all React.lazy-based dynamic imports for navigation screen components across several navigation modules, replacing them with static imports. Additionally, a CI/CD test comment is deleted from the application entry file. No functional code, logic, or exported entity signatures are altered. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Navigation
participant ScreenComponent
App->>Navigation: Initialize navigation stack
Navigation->>ScreenComponent: Import screen statically (at startup)
App->>Navigation: Navigate to Screen
Navigation->>ScreenComponent: Render component immediately
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
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. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/navigation/settings.ts (1)
5-9: Static imports fix the web build but regress mobile startup performanceEagerly importing every settings screen guarantees the bundler has no trouble on the web, but it also eliminates the code-splitting advantage we previously had with
React.lazy, so all five screens now inflate the baseline JS bundle and memory footprint on iOS/Android.A compromise worth considering:
import { Platform } from 'react-native'; import * as React from 'react'; const CloudBackupScreen = Platform.OS === 'web' ? require('../screens/settings/CloudBackupScreen').default : React.lazy(() => import('../screens/settings/CloudBackupScreen'));Apply the same pattern for the other screens. This keeps the web build green while preserving deferred loading on native platforms.
📜 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 (7)
app/App.tsx(0 hunks)app/src/navigation/aesop.ts(1 hunks)app/src/navigation/dev.ts(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)
💤 Files with no reviewable changes (1)
- app/App.tsx
🧰 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/prove.tsapp/src/navigation/recovery.tsapp/src/navigation/dev.tsapp/src/navigation/aesop.tsapp/src/navigation/passport.tsapp/src/navigation/settings.ts
🧠 Learnings (1)
app/src/navigation/dev.ts (1)
Learnt from: transphorm
PR: #795
File: app/android/app/build.gradle:157-158
Timestamp: 2025-07-29T01:08:28.530Z
Learning: For this React Native project, the team prefers build flexibility over fail-fast behavior for release builds in app/android/app/build.gradle. They intentionally allow fallback to debug signing for local development runs, relying on Google Play Console validation to catch any improperly signed releases during upload.
⏰ 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/prove.ts (1)
5-9: LGTM: Static imports resolve web build issues effectively.The conversion from React.lazy dynamic imports to static imports aligns well with the PR objective of fixing web building and loading. This approach ensures all prove screens are bundled upfront, eliminating potential code-splitting issues in web environments.
Trade-off consideration: While this increases the initial bundle size, it provides more predictable loading behavior and resolves the build issues you're addressing.
app/src/navigation/recovery.ts (1)
5-10: Consistent refactoring approach approved.The static import conversion for recovery screens follows the same pattern as other navigation files. Given that recovery screens often form a sequential flow, bundling them together makes logical sense and aligns with the web build fix objective.
app/src/navigation/dev.ts (1)
5-9: Static imports particularly beneficial for dev screens.Converting dev screens to static imports is especially appropriate since these screens are typically used together during development and debugging. This eliminates unnecessary lazy loading complexity in the development environment while maintaining consistency with the overall refactoring approach.
app/src/navigation/passport.ts (1)
5-11: Logical grouping of passport flow screens.The static import approach works well for passport screens since they represent a cohesive user flow (scanning → NFC → troubleshooting). Bundling these related screens together improves the user experience by ensuring immediate availability during the passport verification process.
app/src/navigation/aesop.ts (1)
7-7: Static import maintains feature flag functionality.The conversion to static import works well here since the conditional logic in
getAesopScreens()still controls whether the screen appears in navigation, while the static import ensures the component is available when the feature flag is enabled. This maintains the intended feature flag behavior while resolving build issues.
This reverts commit bf158fd.
* Revert "fix yarn web (#814)" This reverts commit bf158fd. * Revert "add hermes ios engine" This reverts commit f6defcf. * Revert "codex feedback" This reverts commit df35a47. * Revert "merge with dev" This reverts commit 47a8d9d. * Revert "update Gemfile.lock and remove lock when reinstalling" This reverts commit 5f25752. * Revert "test package update" This reverts commit 19dcba0. * Revert "revert install cmd changes" This reverts commit 5427bd1. * Revert "Fix CI build by excluding nokogiri in GitHub Actions/Act environments" This reverts commit dbff69a. * Revert "prettier" This reverts commit 87f4214. * Revert "fix building" This reverts commit fbbefd2. * Revert "optimize path resolving" This reverts commit 23b1118. * Revert "fix loading" This reverts commit f0c884b. * Revert "clean up dupe bundle install" This reverts commit 5428567. * Revert "fix metro loading" This reverts commit 3a76600. * Revert "remove passport provider lazy loading" This reverts commit 5f793a5. * Revert "allow cursor to edit Gemfile and update lock file" This reverts commit b6f7158. * Revert "Update Gemfile.lock to exclude nokogiri in CI environments" This reverts commit 2436401. * Revert "fix install commands" This reverts commit 2dc7d7c. * Revert "fix imports and test" This reverts commit 83d6308. * Revert "fix import" This reverts commit fa42b07. * Revert "update build checks" This reverts commit b281f15. * Revert "save updated imports" This reverts commit 215bca4. * Revert "fix build command" This reverts commit 37f95bc. * Revert "build dependencies before linting" This reverts commit 9e57e01. * Revert "lint suggestions" This reverts commit ff9b9d2. * Revert "fix type. more opportunities" This reverts commit 7ad82d5. * Revert "add typing to crypto loader" This reverts commit d55eec8. * Revert "yarn nice" This reverts commit df1c2db. * Revert "update lock" This reverts commit 04692ba. * Revert "cm feedback" This reverts commit 848071f. * fix file name * fix web loading * fix border width styling * fix package commands * fix import and update lock * fixes from reverted commits * more fixes for web building * fix yarn web:build * add yarn web:build workflow * cm feedback * final fixes * fix for and vitge * fix loading
Summary by CodeRabbit
Refactor
Chores