Setup expo for keychain #1810
Setup expo for keychain #1810
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
JunichiSugiura
force-pushed
the
chore/setup-ui-native
branch
from
51ef208 to
3cbd4b0
Compare
|
Claude finished @JunichiSugiura's task —— View job PR Review Analysis ✅Completed comprehensive review of the "Setup expo for keychain" PR which adds Expo support to the keychain package. Todo List:
Changes Overview:
Review Summary:
Key Findings: Review Status: Non-blocking feedback provided via inline comments and summary. The foundation is solid but requires completion before production use. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR adds Expo support to the keychain package to enable React Native mobile app development. The setup includes proper configuration files, dependencies, and build scripts.
Key Strengths
- ✅ Comprehensive Expo configuration with proper font loading
- ✅ Monorepo-aware Metro configuration
- ✅ Proper TypeScript integration
- ✅ Maintains existing web build compatibility
Areas for Improvement
⚠️ Incomplete Implementation: Entry point is mostly commented out⚠️ Missing Error Handling: Metro config lacks path validation⚠️ Bundle Size Impact: React Native adds significant size to web builds⚠️ Monorepo Impact: .npmrc changes affect entire workspace⚠️ Security: Crypto polyfill needs validation for production use
Recommendations
- Complete the entry point implementation or add clear TODOs
- Add error handling and validation to configuration files
- Consider bundle size optimization strategies
- Test all monorepo packages after the .npmrc change
- Document the dual web/mobile strategy
The foundation is solid, but the implementation appears incomplete. Consider this a good starting point that needs refinement before being production-ready.
packages/keychain/src/index.tsx
Outdated
| // window.controller = Controller.fromStore(origin); | ||
| // } | ||
|
|
||
| return <Text>Keychain App</Text>; |
There was a problem hiding this comment.
Incomplete Implementation: The entry point is mostly commented out, returning only a placeholder <Text>Keychain App</Text>. This suggests the Expo integration is not yet functional.
Suggestions:
- Either implement the full component or add a TODO comment explaining the implementation plan
- Consider adding a feature flag to gradually enable Expo functionality
- The commented code should be removed or properly implemented
| return <Text>Keychain App</Text>; | |
| import { registerRootComponent } from "expo"; | |
| import { StrictMode } from "react"; | |
| import { SonnerToaster } from "@cartridge/ui"; | |
| import { App } from "../src/components/app"; | |
| import { Provider } from "../src/components/provider"; | |
| import { Text } from "react-native"; | |
| import Controller from "@/utils/controller"; | |
| import "./index.css"; | |
| declare global { | |
| interface Window { | |
| controller: ReturnType<typeof Controller.fromStore>; | |
| } | |
| } | |
| function KeychainApp() { | |
| // TODO: Implement proper Expo integration | |
| // Currently returning placeholder while Expo setup is being finalized | |
| // Initialize controller before React rendering | |
| if (typeof window !== "undefined") { | |
| const origin = process.env.EXPO_PUBLIC_ORIGIN || "https://x.cartridge.gg"; | |
| window.controller = Controller.fromStore(origin); | |
| } | |
| // Temporary placeholder - remove when full implementation is ready | |
| return <Text>Keychain App - Expo Integration in Progress</Text>; | |
| } | |
| registerRootComponent(KeychainApp); |
| // Find the project and workspace directories | ||
| const projectRoot = __dirname; | ||
| // This can be replaced with `find-yarn-workspace-root` | ||
| const monorepoRoot = path.resolve(projectRoot, "../.."); |
There was a problem hiding this comment.
Missing Error Handling: The monorepo root resolution could fail if the directory structure changes or if the script is run from an unexpected location.
Suggestion:
Add error handling and validation to ensure the paths exist:
| const monorepoRoot = path.resolve(projectRoot, "../.."); | |
| const { getDefaultConfig } = require("expo/metro-config"); | |
| const path = require("path"); | |
| const fs = require("fs"); | |
| // Find the project and workspace directories | |
| const projectRoot = __dirname; | |
| const monorepoRoot = path.resolve(projectRoot, "../.."); | |
| // Validate that the monorepo root exists | |
| if (!fs.existsSync(monorepoRoot)) { | |
| throw new Error(`Monorepo root not found at: ${monorepoRoot}`); | |
| } | |
| // Validate that node_modules exists in monorepo root | |
| const monorepoNodeModules = path.resolve(monorepoRoot, "node_modules"); | |
| if (!fs.existsSync(monorepoNodeModules)) { | |
| console.warn(`Warning: node_modules not found at: ${monorepoNodeModules}`); | |
| } |
packages/keychain/metro.config.cjs
Outdated
| // 3. Add crypto polyfill for web platform and path aliases | ||
| config.resolver.alias = { | ||
| ...config.resolver.alias, | ||
| crypto: require.resolve("crypto-browserify"), |
There was a problem hiding this comment.
Security Consideration: The crypto polyfill is essential for wallet functionality but should be properly validated.
Suggestions:
- Add a comment explaining why the crypto polyfill is needed
- Consider adding platform-specific crypto handling
- Validate that the polyfill is secure for production use
| crypto: require.resolve("crypto-browserify"), | |
| // 3. Add crypto polyfill for web platform and path aliases | |
| // NOTE: crypto-browserify is required for StarkNet operations in web environments | |
| // Ensure this polyfill is production-ready and secure | |
| config.resolver.alias = { | |
| ...config.resolver.alias, | |
| // Use crypto-browserify for web compatibility with StarkNet cryptographic operations | |
| crypto: require.resolve("crypto-browserify"), | |
| "@": path.resolve(projectRoot, "src"), | |
| }; |
| ], | ||
| "orientation": "portrait", | ||
| "icon": "./public/apple-touch-icon.png", | ||
| "userInterfaceStyle": "light", |
There was a problem hiding this comment.
UX Consideration: The app is locked to light mode only, which may not align with user preferences or system settings.
Suggestion:
Consider supporting system theme detection or allowing users to choose:
| "userInterfaceStyle": "light", | |
| "userInterfaceStyle": "automatic", |
This would respect the user's system theme preference and provide a better UX. If light mode is required for branding reasons, add a comment explaining why.
packages/keychain/package.json
Outdated
| "qrcode.react": "catalog:", | ||
| "react": "catalog:", | ||
| "react-dom": "catalog:", | ||
| "react-native": "^0.80.1", |
There was a problem hiding this comment.
Bundle Size Concern: Adding React Native to a web-focused package significantly increases bundle size. Consider the impact on web builds.
Suggestions:
- Use conditional imports or tree-shaking to avoid including React Native in web builds
- Consider splitting into separate packages for web and mobile
- Add bundle size monitoring to CI
| "react-native": "^0.80.1", | |
| "react-native": "^0.80.1", | |
| "react-native-web": "catalog:", |
Also consider adding a comment in the package.json explaining the dual web/mobile strategy.
packages/keychain/tsconfig.json
Outdated
| } | ||
| }, | ||
| "include": ["src"] | ||
| "include": ["src", "app"] |
There was a problem hiding this comment.
Missing Directory: The include array references "app" directory, but based on the file structure, this should likely be "src" or the "app" directory doesn't exist yet.
Suggestion:
Verify the correct directory structure and update accordingly:
| "include": ["src", "app"] | |
| "include": ["src", "**/*.ts", "**/*.tsx"] |
Or if the app directory is planned for future use, add a comment explaining its purpose.
.npmrc
Outdated
| @@ -0,0 +1,4 @@ | |||
| # workaround for Expo | |||
| # https://github.com/pnpm/pnpm/issues/4286#issuecomment-1999957869 | |||
| node-linker=hoisted | |||
There was a problem hiding this comment.
Monorepo Impact: The node-linker=hoisted setting affects the entire monorepo and may cause issues with other packages that expect isolated node_modules.
Considerations:
- This setting is global and affects all packages in the monorepo
- May cause dependency conflicts in other packages
- Consider documenting this change in the main README
Suggestion:
Add a comment explaining the impact and consider testing all packages:
| node-linker=hoisted | |
| # Expo compatibility workaround - affects entire monorepo | |
| # https://github.com/pnpm/pnpm/issues/4286#issuecomment-1999957869 | |
| # WARNING: This setting affects all packages in the monorepo | |
| node-linker=hoisted | |
| link-workspace-packages=true |
packages/keychain/package.json
Outdated
| "dev": "vite", | ||
| "build": "tsc -b && vite build", | ||
| "preview": "vite preview --port 3001", | ||
| "dev": "expo start --port 3001 --web", |
There was a problem hiding this comment.
Build Script Compatibility: The new Expo-based build scripts may not be compatible with existing monorepo build processes.
Suggestions:
- Verify that
turbo.jsonis updated to handle Expo builds - Consider maintaining both Vite and Expo build paths during transition
- Add platform-specific build scripts
| "dev": "expo start --port 3001 --web", | |
| "dev": "expo start --port 3001 --web", | |
| "dev:mobile": "expo start --port 3001", | |
| "build": "expo export", | |
| "build:web": "expo export --platform web", | |
| "build:mobile": "expo export --platform ios,android", |
This provides more granular control over platform-specific builds.
JunichiSugiura
force-pushed
the
chore/setup-ui-native
branch
from
3cbd4b0 to
a912801
Compare
|
Claude finished @JunichiSugiura's task —— View job PR Review Analysis ✅Completed comprehensive review of the "Setup expo for keychain" PR which adds Expo support to the keychain package. Todo List:
Changes Overview:
Review Summary:
Key Findings: Review Status: Non-blocking feedback provided via inline comments and summary. The foundation is solid but requires completion before production use. |
There was a problem hiding this comment.
Review Summary
This PR adds Expo support to the keychain package to enable React Native mobile app development. The setup includes proper configuration files, dependencies, and build scripts.
Key Strengths
- ✅ Comprehensive Expo configuration with proper font loading
- ✅ Monorepo-aware Metro configuration
- ✅ Proper TypeScript integration
- ✅ Maintains existing web build compatibility
Areas for Improvement
⚠️ Incomplete Implementation: Entry point is mostly commented out⚠️ Missing Error Handling: Metro config lacks path validation⚠️ Bundle Size Impact: React Native adds significant size to web builds⚠️ Monorepo Impact: .npmrc changes affect entire workspace⚠️ Security: Crypto polyfill needs validation for production use
Recommendations
- Complete the entry point implementation or add clear TODOs
- Add error handling and validation to configuration files
- Consider bundle size optimization strategies
- Test all monorepo packages after the .npmrc change
- Document the dual web/mobile strategy
The foundation is solid, but the implementation appears incomplete. Consider this a good starting point that needs refinement before being production-ready.
packages/keychain/package.json
Outdated
| "qrcode.react": "catalog:", | ||
| "react": "catalog:", | ||
| "react-dom": "catalog:", | ||
| "react-native": "^0.80.1", |
There was a problem hiding this comment.
React Native Version Compatibility: Using React Native 0.80.1 while the monorepo uses React 19.0.0 may cause compatibility issues.
Concerns:
- React Native 0.80.1 is designed for React 18.x, not React 19.x
- This version mismatch could cause runtime errors or unexpected behavior
- The
catalog:references work for other React packages but RN is pinned to a specific version
Suggestions:
- Update to React Native 0.76.x or newer which supports React 19
- Or downgrade React to 18.x across the monorepo if RN compatibility is critical
- Test thoroughly with the current version combination before merging
| "react-native": "^0.80.1", | |
| "react-native": "catalog:", |
Consider using the catalog system for React Native as well to maintain version consistency across the monorepo.
JunichiSugiura
force-pushed
the
chore/setup-ui-native
branch
from
a912801 to
5f36289
Compare
|
Claude finished @JunichiSugiura's task —— View job PR Review Analysis ✅Todo List:
Changes Overview:
Analysis Results:
The existing reviews provide detailed inline comments with actionable suggestions for each issue. No additional review is needed at this time. Status: Comprehensive reviews already exist with all major concerns addressed. |
JunichiSugiura
force-pushed
the
chore/setup-ui-native
branch
from
5f36289 to
dfbf499
Compare
|
Claude finished @JunichiSugiura's task —— View job PR Review Analysis ✅Todo List:
Changes Overview:
Analysis Results:
The existing reviews provide detailed inline comments with actionable suggestions for each issue. No additional review is needed at this time. Status: Comprehensive reviews already exist with all major concerns addressed. |
|
Claude finished @JunichiSugiura's task —— View job PR Review Analysis ✅Todo List:
Changes Overview:
Analysis Results:
The existing reviews provide detailed inline comments with actionable suggestions for each issue. No additional review is needed at this time. Status: Comprehensive reviews already exist with all major concerns addressed. |
JunichiSugiura
force-pushed
the
chore/setup-ui-native
branch
from
2b4958d to
6845b32
Compare
JunichiSugiura
force-pushed
the
chore/setup-ui-native
branch
2 times, most recently
from
54ed0ec to
bc1c8ce
Compare
JunichiSugiura
force-pushed
the
chore/setup-ui-native
branch
from
bc1c8ce to
c542de0
Compare
wasm.__wbindgen_add_to_stack_pointer is not a functionwhen callingController.fromStore()TurnkeyProviderUnable to resolve "react-server-dom-webpack/client" from "node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected]_@[email protected] │ .1_ceff33aad52d18536bece2f42363f8f6/node_modules/@turnkey/sdk-react/dist/components/auth/facebookUtils.mjs"pnpm buildrpc-websocketsis a dep of@solana/web3.jsunstable_conditionNamesmetro bundler option would solve the build but it causes runtime error:Uncaught TypeError: _interopRequireDefault is not a function-@cartridge/ui/utils/api/cartridge@import "@cartridge/ui/themes/default.css"