chore: new example app + action improvements #483
Conversation
|
|
Claude finished @ignaciosantise's task —— View job 🔍 Code Review CompleteI've reviewed this PR focusing on code quality, security, and best practices. Here are my findings: Critical Issues1. Security: Project ID Validation Logic Differs Between AppsLocation: The new example uses const projectId = process.env.EXPO_PUBLIC_PROJECT_ID ?? 'undefined';
if (projectId === 'undefined') {
Alert.alert('Project ID is not set');
}However, the apps/native app uses an empty string: const projectId = process.env.EXPO_PUBLIC_PROJECT_ID ?? '';Issue: The expo-multichain example will pass Recommendation: Use consistent validation across examples and throw an error or return early instead of just showing an alert: const projectId = process.env.EXPO_PUBLIC_PROJECT_ID;
if (!projectId || projectId === 'undefined') {
throw new Error('EXPO_PUBLIC_PROJECT_ID must be set in .env file');
}High Priority Issues2. GitHub Actions: Unsafe Branch Name TransformationLocation: snapshot=$(git branch --show-current | tr -cs '[:alnum:]-' '-' | tr '[:upper:]' '[:lower:]' | sed 's/-$//')Issue: This transformation can create invalid npm tags or collisions:
Recommendation: Add validation to ensure the snapshot tag is a valid npm tag and consider using a hash to prevent collisions: snapshot=$(git branch --show-current | tr -cs '[:alnum:]-' '-' | tr '[:upper:]' '[:lower:]' | sed 's/-$//' | sed 's/^-//')
# Validate it's not empty
if [ -z "$snapshot" ]; then
echo "::error::Invalid branch name for snapshot tag"
exit 1
fi3. GitHub Actions: Missing Error Handling in PR Comment StepLocation: PR_NUMBER=$(gh api repos/${{ github.repository }}/commits/${{ github.sha }}/pulls \
--jq '.[0].number')
if [ -n "$PR_NUMBER" ] && [ "$PR_NUMBER" != "null" ]; thenIssue: If the API call fails, Recommendation: Add explicit error handling: PR_NUMBER=$(gh api repos/${{ github.repository }}/commits/${{ github.sha }}/pulls \
--jq '.[0].number' 2>/dev/null || echo "")
if [ -n "$PR_NUMBER" ] && [ "$PR_NUMBER" != "null" ] && [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then4. React Native Reanimated Dependency ViolationLocation: The new example includes
Impact: This contradicts the established architecture principles and could lead to:
Recommendation: Remove Medium Priority Issues5. Maestro Test: Hardcoded Wallet IDLocation: - assertVisible:
id: "wallet-search-item-6880782cf8ff712bf8772b585960346290fd2499c1f1c51df9fe713d9d2e9384"Issue: This hardcoded ID appears to be a hash. If the wallet registry updates or IDs change, this test will break silently. Recommendation:
6. Maestro Test: Flaky Double TapLocation: - doubleTapOn:
id: "wallet-search-item-6880782cf8ff712bf8772b585960346290fd2499c1f1c51df9fe713d9d2e9384"
delay: 200Issue: Using Recommendation: Use a single 7. Missing .env File in .gitignoreLocation: ✅ Good: However, the pattern Verification needed: Ensure 8. Inconsistent Storage Implementation PatternLocation: The storage implementation uses Recommendation: Add try-catch blocks for getItem: async <T = any>(key: string): Promise<T | undefined> => {
try {
const item = await AsyncStorage.getItem(key);
if (typeof item === 'undefined' || item === null) {
return undefined;
}
return safeJsonParse(item) as T;
} catch (error) {
console.warn(`Failed to get item ${key}:`, error);
return undefined;
}
}9. Type Safety: Unsafe Type AssertionLocation: const wagmiAdapter = new WagmiAdapter({
projectId,
networks: networks as any
});Issue: Using Recommendation: Either fix the type mismatch properly or add a comment explaining why the type assertion is necessary. Low Priority / Suggestions10. ESLint Configuration: Overly Broad Ignore PatternLocation: "ignorePatterns": [
"examples/"
]Issue: Ignoring the entire Recommendation: Consider linting example apps but with a separate, more relaxed config. Examples should demonstrate best practices. 11. Maestro README: Inconsistent Test File ReferenceLocation: - `connect-wallet-detailed.yaml` - Detailed wallet connection flow testBut the actual file is named Fix: Update the README to match the actual filename. 12. Missing Test Script in Example Package.jsonThe example has "maestro:test": "bash .maestro/run-tests.sh"13. Code Formatting: Unnecessary ChangesMany of the formatting changes in Suggestion: Consider running formatters separately from feature work in future PRs to keep diffs focused. Positive Observations✅ Good: Comprehensive Maestro test setup with screenshots and video recording SummaryCritical: 1 issue (project ID validation) The PR introduces valuable E2E testing infrastructure and a new example app, but needs attention to security validation logic and workflow safety before merging. The react-native-reanimated dependency should be reviewed against project architecture guidelines. |
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a comprehensive new Expo Multichain example application along with improvements to GitHub Actions workflows and code formatting enhancements. The PR focuses on expanding the project's testing capabilities, workflow automation, and maintaining code quality through consistent formatting.
- Introduces a complete Expo Multichain example app with Maestro E2E testing infrastructure
- Enhances GitHub Actions workflows for better snapshot release management and changeset handling
- Applies consistent code formatting and style improvements across the native app codebase
Reviewed Changes
Copilot reviewed 51 out of 64 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/expo-multichain/* | New complete Expo multichain example app with UI components, configuration, and app structure |
| .maestro/* | Maestro E2E test flows and documentation for automated UI testing |
| .github/workflows/* | Enhanced snapshot release workflow with custom tags and improved changeset PR detection |
| apps/native/src/views/* | Code formatting improvements and JSX restructuring for better readability |
| .eslintrc.json | Updated to ignore examples directory from linting |
Comments suppressed due to low confidence (1)
examples/expo-multichain/run-tests.sh:1
- The script references 'maestro-qr-load.yaml' but the actual file is named 'wallet-qr-load.yaml'. This will cause the test to fail.
#!/bin/bash
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@claude review |
|
Claude finished @ignaciosantise's task —— View job 🔄 Incremental Code Review CompleteI've analyzed the latest code against the previous review from October 9th. Here are my findings on issue status and new concerns: ✅ Issues Resolved Since Last Review1. Project ID Validation - RESOLVEDPrevious Location: The critical security issue has been fixed. The code now properly validates the project ID: const projectId = process.env.EXPO_PUBLIC_PROJECT_ID ?? '';
if (projectId === '') {
Alert.alert('Project ID is not set');
}The previous issue where it would pass 2. Workflow Branch Transformation - IMPROVEDLocation: The unsafe branch name transformation has been improved with validation: snapshot=$(git branch --show-current | tr -cs '[:alnum:]-' '-' | tr '[:upper:]' '[:lower:]' | sed 's/-$//' | sed 's/^-//')
# Validate it's not empty
if [ -z "$snapshot" ]; then
echo "::error::Invalid branch name for snapshot tag"
exit 1
fiThis addresses the previous concern about invalid npm tags. 3. Changeset Workflow Error Handling - IMPROVEDLocation: Better error handling has been added: PR_NUMBER=$(gh api repos/${{ github.repository }}/commits/${{ github.sha }}/pulls \
--jq '.[0].number' 2>/dev/null || echo "")
if [ -n "$PR_NUMBER" ] && [ "$PR_NUMBER" != "null" ]; thenThe
|
This pull request introduces several improvements and additions to the project, focusing on workflow enhancements, code formatting consistency, and expanded testing for the Expo Multichain example app. The most significant changes include enhancements to the GitHub Actions workflows for snapshot releases, the addition of Maestro end-to-end tests and documentation for the Expo Multichain example, and minor code cleanups for better readability and maintainability.
GitHub Actions Workflow Improvements:
.github/workflows/snapshot.yml) to support custom snapshot tags, output the published tag, and skip publishing if no changesets are found. Also, improved branch/tag handling and outputs for better CI integration. [1] [2] [3].github/workflows/changesets.yml) to more reliably find and comment on pull requests associated with the current commit, with better handling for direct pushes to main.End-to-End Testing and Example App Enhancements:
.maestro/basic-smoke-test.yaml), documentation (.maestro/README.md), and updated.gitignorefor theexamples/expo-multichainapp, enabling automated UI testing and improving local development and CI reliability. [1] [2] [3].env.examplefile for the Expo Multichain example to clarify required environment variables.Code Formatting and Minor Cleanups:
examples/directory, reducing unnecessary linting noise.apps/nativedirectory for improved readability and maintainability (e.g., splitting imports, fixing trailing semicolons, and formatting JSX). [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16]Prettier Ignore Update:
.prettierignoreto ensure proper formatting exclusions.Note
Introduces a new Expo Multichain example app (Expo Router) with Maestro test flows and minor native tweaks, including conditional gas estimation in Wagmi actions.
examples/expo-multichainwith Expo Router setup, UI components, storage, theming, and multichain AppKit (Ethereum/Wagmi, Solana, Bitcoin) integration.app.json), TypeScript config, Babel/ESLint configs, assets, and environment template (.env.example)..maestrotest flows (smoke, QR load, connect-wallet), docs, and arun-tests.shscript; updates.gitignorefor artifacts.apps/native/src/views/WagmiActionsView.tsx: AddsuseAccountand enablesuseEstimateGasonly whenisConnected.Written by Cursor Bugbot for commit c9694a3. This will update automatically on new commits. Configure here.