chore(ci): refactor test-nitro-cli workflow#328
Conversation
perms: reduce to read-only support workflow_dispatch matrix: fail-fast=false rm pnpm setup node only for yarn add pods/gradle cache env-based iOS scheme
WalkthroughUpdates CI workflows and scripts: adjusts permissions to read-only, broadens triggers, switches macOS runner, adds Xcode setup for E2E iOS, introduces CocoaPods and Gradle caching, simplifies Node.js gating (removes pnpm), adds SCHEME env for iOS builds, downloads upstream artifacts, and adds explicit iOS/Android build failure checks in the Maestro script. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Upstream Workflow/Dispatch
participant GHA as GitHub Actions (test-nitro-cli.yml)
participant Cache as Actions Cache
participant Art as Artifact Store
participant Xcode as xcodebuild
participant Gradle as Gradle
Dev->>GHA: Trigger (workflow_run success or workflow_dispatch)
rect rgba(200,230,255,0.3)
note over GHA: iOS Job
GHA->>Art: Download generated package
GHA->>Cache: Restore CocoaPods cache
GHA->>Xcode: Build with SCHEME (env)
alt Build succeeds
Xcode-->>GHA: 0
else Build fails
Xcode-->>GHA: non-zero
GHA-->>GHA: Job fails
end
end
rect rgba(200,255,200,0.3)
note over GHA: Android Job
GHA->>Cache: Restore Gradle cache
GHA->>Gradle: assembleRelease
alt Build succeeds
Gradle-->>GHA: 0
else Build fails
Gradle-->>GHA: non-zero
GHA-->>GHA: Job fails
end
end
sequenceDiagram
autonumber
participant GHA as GitHub Actions (e2e-tests.yml)
participant XSetup as setup-xcode@v1
participant Steps as Subsequent iOS steps
GHA->>XSetup: Install Xcode 16.4
alt Setup succeeds
XSetup-->>GHA: OK
GHA->>Steps: Continue (download/codegen/bun/pods/maestro)
else Setup fails
XSetup-->>GHA: Error
GHA-->>GHA: Fail early
end
sequenceDiagram
autonumber
participant Script as scripts/e2e-maestro.sh
participant Xcode as xcodebuild|xcpretty
participant Gradle as ./gradlew
Script->>Xcode: iOS build (pipe to xcpretty if present)
alt Non-zero exit
Xcode-->>Script: Failure
Script-->>Script: Print "❌ iOS build failed!" and exit 1
else Zero exit
Xcode-->>Script: Success
Script->>Gradle: Android assembleRelease
alt Non-zero exit
Gradle-->>Script: Failure
Script-->>Script: Print "❌ Android build failed!" and exit 1
else Zero exit
Gradle-->>Script: Success
Script-->>Script: Continue
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test-nitro-cli.yml (1)
38-46: Manual runs will fail: run-id is undefined on workflow_dispatch.The download step dereferences github.event.workflow_run.id when manually dispatched, causing failure; the rest of the job also needs the artifact. Add event-guarded download steps and require a run_id input for workflow_dispatch.
- - name: Download generated package - uses: actions/download-artifact@v5 - with: - name: test-${{ matrix.package-type }}-${{ matrix.pm }} - path: ${{ env.WORKING_DIR }} - run-id: ${{ github.event.workflow_run.id }} - github-token: ${{ github.token }} - repository: ${{ github.repository }} + - name: Download generated package (from workflow_run) + if: github.event_name == 'workflow_run' + uses: actions/download-artifact@v5 + with: + name: test-${{ matrix.package-type }}-${{ matrix.pm }} + path: ${{ env.WORKING_DIR }} + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ github.token }} + repository: ${{ github.repository }} + + - name: Download generated package (from manual input) + if: github.event_name == 'workflow_dispatch' + uses: actions/download-artifact@v5 + with: + name: test-${{ matrix.package-type }}-${{ matrix.pm }} + path: ${{ env.WORKING_DIR }} + run-id: ${{ github.event.inputs.run_id }} + github-token: ${{ github.token }} + repository: ${{ github.repository }}Add the input to workflow_dispatch (outside this hunk):
on: workflow_dispatch: inputs: run_id: description: 'Run ID of the upstream "Generate Packages" workflow' required: true type: string
🧹 Nitpick comments (5)
.github/workflows/test-nitro-cli.yml (5)
22-24: Condition is fine; consider being explicit for workflow_run.Optional: guard references to github.event.workflow_run.* behind an explicit event check for readability.
- if: github.event_name == 'workflow_dispatch' || github.event.workflow_run.conclusion == 'success' + if: github.event_name == 'workflow_dispatch' || (github.event_name == 'workflow_run' && github.event.workflow_run.conclusion == 'success')
31-34: SCHEME env: OK, but workspace coupling may bite later.If workspace name ever diverges from scheme, introduce WORKSPACE_NAME to decouple.
env: WORKING_DIR: ${{ github.workspace }}/react-native-test-${{ matrix.package-type }} - SCHEME: ${{ matrix.package-type == 'module' && 'TestModuleExample' || 'TestViewExample' }} + SCHEME: ${{ matrix.package-type == 'module' && 'TestModuleExample' || 'TestViewExample' }} + WORKSPACE_NAME: ${{ env.SCHEME }} # change if workspace != schemeAnd use "${WORKSPACE_NAME}.xcworkspace" below.
96-105: Pod cache key may be empty on cold starts; broaden restore key.hashFiles on Podfile.lock can be empty before install. Include Podfile in the key or keep as-is but add an additional restore key for the workspace to improve hit rate.
- key: ${{ runner.os }}-pods-${{ hashFiles(format('{0}/example/ios/Podfile.lock', env.WORKING_DIR)) }} - restore-keys: | - ${{ runner.os }}-pods- + key: ${{ runner.os }}-pods-${{ hashFiles(format('{0}/example/ios/Podfile.lock', env.WORKING_DIR)) }}-${{ hashFiles(format('{0}/example/ios/Podfile', env.WORKING_DIR)) }} + restore-keys: | + ${{ runner.os }}-pods- + ${{ runner.os }}-pods-${{ hashFiles(format('{0}/example/ios/Podfile', env.WORKING_DIR)) }}-
160-165: Node is required later even for bun path.node post-script.js runs regardless of pm; rely on runner Node or install explicitly for predictability.
- - name: Setup Node.js - if: matrix.pm == 'yarn' + - name: Setup Node.js + # Node needed for yarn and for node post-script.jsIf you prefer keeping it conditional, add a lightweight check step to echo node -v on bun path to ensure availability.
203-212: Duplicate Gradle caching with setup-java.actions/setup-java@v5 with cache: gradle already configures the Gradle cache. Keeping both can lead to unnecessary cache churn. Pick one.
- - name: Cache Gradle - uses: actions/cache@v4 - with: - path: | - ~/.gradle/caches - ~/.gradle/wrapper - key: ${{ runner.os }}-gradle-${{ hashFiles(format('{0}/example/android/**/*.gradle*', env.WORKING_DIR)) }} - restore-keys: | - ${{ runner.os }}-gradle- + # Rely on setup-java cache: gradle above, or remove 'cache: gradle' there and keep this block — but not both.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/test-nitro-cli.yml(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.github/workflows/test-nitro-cli.yml (1)
src/generate-nitro-package.ts (2)
setupWorkflows(475-499)configureExamplePackageJson(303-343)
🔇 Additional comments (2)
.github/workflows/test-nitro-cli.yml (2)
3-6: Least-privilege perms look good.contents: read and actions: read are sufficient for cross-run artifact download; nice hardening.
114-127: Verify workspace/scheme exist to fail fast with diagnostics.Add a quick list step before build so misnamed schemes/workspaces are obvious in logs.
- name: Build iOS project working-directory: ${{ env.WORKING_DIR }}/example/ios - run: | + run: | + echo "Available workspaces:" && ls -1 *.xcworkspace || true + echo "Available schemes:" && xcodebuild -list -json | jq -r '.workspace.schemes[]?' || true set -o pipefail && xcodebuild \ CC=clang CPLUSPLUS=clang++ LD=clang LDPLUSPLUS=clang++ \ -derivedDataPath build -UseModernBuildSystem=YES \ - -workspace "${SCHEME}.xcworkspace" \ + -workspace "${WORKSPACE_NAME:-${SCHEME}}.xcworkspace" \ -scheme "${SCHEME}" \ -sdk iphonesimulator \ -configuration ${{ matrix.mode }} \ -destination 'platform=iOS Simulator,name=iPhone 16' \ build \ CODE_SIGNING_ALLOWED=NO | xcpretty
| test-android-build: | ||
| name: Test Android Build - ${{ matrix.pm }} - ${{ matrix.package-type }} (${{ matrix.mode }}) | ||
| if: github.event.workflow_run.conclusion == 'success' | ||
| # Run on successful upstream, or when manually dispatched | ||
| if: github.event_name == 'workflow_dispatch' || github.event.workflow_run.conclusion == 'success' | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Same artifact issue on Android manual runs.
Mirror the iOS fix: add event-guarded download steps and workflow_dispatch input usage.
- if: github.event_name == 'workflow_dispatch' || github.event.workflow_run.conclusion == 'success'
+ if: github.event_name == 'workflow_dispatch' || (github.event_name == 'workflow_run' && github.event.workflow_run.conclusion == 'success')And apply the two-step download pattern as in Lines 38-46 for Lines 145-153.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test-android-build: | |
| name: Test Android Build - ${{ matrix.pm }} - ${{ matrix.package-type }} (${{ matrix.mode }}) | |
| if: github.event.workflow_run.conclusion == 'success' | |
| # Run on successful upstream, or when manually dispatched | |
| if: github.event_name == 'workflow_dispatch' || github.event.workflow_run.conclusion == 'success' | |
| runs-on: ubuntu-latest | |
| test-android-build: | |
| name: Test Android Build - ${{ matrix.pm }} - ${{ matrix.package-type }} (${{ matrix.mode }}) | |
| # Run on successful upstream, or when manually dispatched | |
| if: github.event_name == 'workflow_dispatch' || (github.event_name == 'workflow_run' && github.event.workflow_run.conclusion == 'success') | |
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/e2e-tests.yml (1)
42-49: Explicit permissions required to download artifacts from another workflow.actions/download-artifact with run-id needs actions: read (and contents: read). Without an explicit permissions block, org-level restrictive defaults can cause 403s.
name: E2E Test with Maestro on: workflow_run: workflows: ['Generate Packages'] types: - completed workflow_dispatch: +permissions: + actions: read + contents: read + concurrency: group: ${{ github.workflow }}-${{ github.event.workflow_run.id || github.run_id }} cancel-in-progress: truescripts/e2e-maestro.sh (1)
44-46: Fragile “iPhone 16” UDID extraction; add fallback and case-insensitive UDID regex.If no “iPhone 16” exists, iphone16Id becomes empty and xcodebuild fails. Also, UDIDs can include lowercase hex.
- iphone16Id=$(xcrun simctl list devices | grep "iPhone 16 (" | grep -E '\(Booted\)|\(Shutdown\)' | head -1 | grep -E -o '\([0-9A-F-]{36}\)' | tr -d '()') - echo "📱 Using iPhone 16 simulator with ID: $iphone16Id" + iphone16Id=$(xcrun simctl list devices | grep "iPhone 16 (" | grep -E '\(Booted\)|\(Shutdown\)' | head -1 | grep -E -o '\([0-9a-fA-F-]{36}\)' | tr -d '()') + if [ -z "${iphone16Id:-}" ]; then + echo "⚠️ No iPhone 16 simulator found; falling back to latest available iPhone device." + iphone16Id=$(xcrun simctl list devices | grep -E 'iPhone [0-9]+' | grep -E '\(Booted\)|\(Shutdown\)' | head -1 | grep -E -o '\([0-9a-fA-F-]{36}\)' | tr -d '()') + fi + if [ -z "${iphone16Id:-}" ]; then + echo "❌ Could not resolve a simulator UDID. Ensure a simulator is available for the installed Xcode runtime." + exit 1 + fi + echo "📱 Using simulator with ID: $iphone16Id"
🧹 Nitpick comments (5)
.github/workflows/e2e-tests.yml (1)
10-12: Safer concurrency key for workflow_run events.Using github.ref under workflow_run can collide or be empty. Prefer the triggering run id to avoid unintended cancellations across branches.
concurrency: - group: ${{ github.workflow }}-${{ github.ref }} + group: ${{ github.workflow }}-${{ github.event.workflow_run.id || github.run_id }} cancel-in-progress: truescripts/e2e-maestro.sh (4)
81-89: Replace fixed sleep with simulator boot status wait.Avoid flaky timing by waiting until the simulator reports “Booted”.
- # Launch the simulator if not already booted - if ! xcrun simctl list devices | grep "$iphone16Id" | grep -q "Booted"; then - echo "🚀 Booting simulator..." - xcrun simctl boot $iphone16Id - else - echo "✅ Simulator already booted" - fi - # Wait for 10 seconds - sleep 10 + # Launch the simulator if not already booted and wait until ready + if ! xcrun simctl list devices | grep "$iphone16Id" | grep -q "Booted"; then + echo "🚀 Booting simulator..." + xcrun simctl boot "$iphone16Id" + else + echo "✅ Simulator already booted" + fi + echo "⏳ Waiting for simulator to be fully booted..." + xcrun simctl bootstatus "$iphone16Id" -b
41-41: Quote paths to handle spaces.Harden cd against spaces in EXAMPLE_DIR.
- cd $EXAMPLE_DIR/ios + cd "$EXAMPLE_DIR/ios"
97-97: Quote paths to handle spaces.Same nit for Android path.
- cd $EXAMPLE_DIR/android + cd "$EXAMPLE_DIR/android"
25-33: Allow SCHEME override via environment for consistency with workflows.Let CI pass SCHEME when needed; fall back to PACKAGE_TYPE defaults.
-APP_ID="" -SCHEME="" +APP_ID="" +SCHEME="${SCHEME:-}" if [ "$PACKAGE_TYPE" == "module" ]; then APP_ID="com.testmoduleexample" - SCHEME="TestModuleExample" + SCHEME="${SCHEME:-TestModuleExample}" elif [ "$PACKAGE_TYPE" == "view" ]; then APP_ID="com.testviewexample" - SCHEME="TestViewExample" + SCHEME="${SCHEME:-TestViewExample}" else echo "Error! You must pass either 'module' or 'view'" echo "" exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/e2e-tests.yml(1 hunks)scripts/e2e-maestro.sh(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.github/workflows/e2e-tests.yml (1)
src/generate-nitro-package.ts (1)
setupWorkflows(475-499)
🔇 Additional comments (4)
.github/workflows/e2e-tests.yml (1)
124-128: Verify Xcode 16.4 availability on macOS-15 and matching simulator runtime.macOS-15 runners may not always have Xcode 16.4 pre-provisioned. Also, the e2e script targets an “iPhone 16” simulator; ensure that Xcode 16.4 provides the required iOS runtime/device. If not present, this step or downstream builds will fail.
Would you like me to add a fallback to a supported Xcode version and/or parameterize the version via an input?
scripts/e2e-maestro.sh (3)
65-71: LGTM: explicit iOS build failure handling after xcpretty.Great addition; prevents silent green pipelines when xcodebuild fails behind the pipe.
74-77: LGTM: explicit iOS build failure handling without xcpretty.Consistent exit-on-fail with a clear message.
102-106: LGTM: Android build failure handling.Clear failure messaging with non-zero exit.
patrickkabwe
deleted the
chore/ci-refactor-test-nitro-cli-20250829-205119
branch
1 participant
This PR refactors the test workflow for safety and speed.
Summary by CodeRabbit