feat: add package type, platforms, and languages options#471
Merged
patrickkabwe merged 5 commits intomainfrom Mar 16, 2026
Merged
feat: add package type, platforms, and languages options#471patrickkabwe merged 5 commits intomainfrom
patrickkabwe merged 5 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughConverts static per-package CI matrices into a dynamic define-matrix-driven workflow; adds CLI flags for platforms and languages with parsing/validation and UI updates; updates iOS build action and E2E script to discover workspace, scheme, and module/app identifiers at runtime. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Define as define-matrix job
participant Gen as Generator jobs
participant Build as Build jobs (iOS/Android)
participant E2E as E2E jobs
participant Art as Artifact storage
GH->>Define: start define-matrix
Define-->>GH: emit matrices (generation, ios_build, android_build, ios_e2e, android_e2e)
GH->>Gen: for each generation matrix entry (package_dir, package_type, pm, scenario)
Gen->>Art: upload generated package artifacts
GH->>Build: use build matrices (from define outputs) to run platform builds
Build->>Art: upload build artifacts (apk/ipa)
GH->>E2E: run e2e jobs using artifacts and discovered workspace/scheme/module IDs
E2E->>Art: upload e2e recordings/logs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/e2e-maestro.sh (1)
85-96:⚠️ Potential issue | 🟡 MinorQuote variables in xcodebuild command to prevent word splitting.
The
$SCHEMEvariable is used unquoted in thebuildCmdstring on lines 86-87. IfSCHEMEcontains spaces (unlikely but possible with custom workspace names), the command will break.🛡️ Proposed fix
buildCmd="xcodebuild \ - -workspace $SCHEME.xcworkspace \ - -scheme $SCHEME \ + -workspace \"$SCHEME.xcworkspace\" \ + -scheme \"$SCHEME\" \ -configuration Release \ - -destination id=$iphone16Id \ + -destination \"id=$iphone16Id\" \ -derivedDataPath build \ -jobs $(sysctl -n hw.ncpu) \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/e2e-maestro.sh` around lines 85 - 96, The xcodebuild command string assigned to buildCmd uses unquoted variable expansions (notably $SCHEME and the workspace expansion $SCHEME.xcworkspace) which can cause word-splitting if the scheme/workspace name contains spaces; update the buildCmd construction to quote the variable expansions (e.g., "$SCHEME" and "$SCHEME.xcworkspace") and any other interpolations like "$iphone16Id" to ensure the command tokens remain intact when executed via eval or spawned.
🧹 Nitpick comments (1)
.github/workflows/ci-packages.yml (1)
74-79: Output key naming inconsistency between definition and reference.The outputs are defined with hyphens (
ios-build,android-build, etc.) but the Node.js script outputs use underscores (ios_build,android_build). GitHub Actions converts hyphenated output names, but this could cause confusion.Looking at line 342:
matrix: ${{ fromJson(needs.define-matrix.outputs.ios-build) }}- this referencesios-build, which maps to theios_buildconsole.log output due to GitHub's output key normalization. This works but is confusing.Consider using consistent naming (either all hyphens or all underscores) for clarity:
♻️ Suggested consistency improvement
outputs: generation: ${{ steps.set-matrix.outputs.generation }} - ios-build: ${{ steps.set-matrix.outputs.ios_build }} - android-build: ${{ steps.set-matrix.outputs.android_build }} - ios-e2e: ${{ steps.set-matrix.outputs.ios_e2e }} - android-e2e: ${{ steps.set-matrix.outputs.android_e2e }} + ios_build: ${{ steps.set-matrix.outputs.ios_build }} + android_build: ${{ steps.set-matrix.outputs.android_build }} + ios_e2e: ${{ steps.set-matrix.outputs.ios_e2e }} + android_e2e: ${{ steps.set-matrix.outputs.android_e2e }}Then update references like
needs.define-matrix.outputs.ios-buildtoneeds.define-matrix.outputs.ios_build.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-packages.yml around lines 74 - 79, The outputs block defines keys with hyphens (generation, ios-build, android-build, ios-e2e, android-e2e) but the Node script and later references use underscores (ios_build, android_build, ios_e2e, android_e2e); rename the output keys to use underscores (e.g., ios_build, android_build, ios_e2e, android_e2e) and update all references that read from the step outputs (e.g., change needs.define-matrix.outputs.ios-build to needs.define-matrix.outputs.ios_build and similarly for android and e2e keys, and ensure any usage of set-matrix.outputs matches the new underscore names) so naming is consistent across the outputs and their consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/e2e-maestro.sh`:
- Around line 60-65: The script does a cd to "$EXAMPLE_DIR/ios" without checking
for failure, so if the directory is missing the rest of the logic
(WORKSPACE_NAME/SCHEME) will run in the wrong path; update the cd invocation
(the line using cd "$EXAMPLE_DIR/ios") to check its exit status and abort (e.g.,
cd ... || { echo "failed to cd to $EXAMPLE_DIR/ios" >&2; exit 1; }) so the
script stops on failure and avoids SC2164 warnings, leaving the subsequent
WORKSPACE_NAME and SCHEME logic unchanged.
- Line 134: The cd command that changes into the Android example directory (the
line containing cd "$EXAMPLE_DIR/android") must handle failure; update that
invocation to check its exit status and abort with a descriptive error message
if it fails (for example, append a fallback that logs the failure and exits or
test the return code and exit 1). Mirror the same failure-handling approach used
for the iOS cd path so the script does not continue when the directory change
fails.
---
Outside diff comments:
In `@scripts/e2e-maestro.sh`:
- Around line 85-96: The xcodebuild command string assigned to buildCmd uses
unquoted variable expansions (notably $SCHEME and the workspace expansion
$SCHEME.xcworkspace) which can cause word-splitting if the scheme/workspace name
contains spaces; update the buildCmd construction to quote the variable
expansions (e.g., "$SCHEME" and "$SCHEME.xcworkspace") and any other
interpolations like "$iphone16Id" to ensure the command tokens remain intact
when executed via eval or spawned.
---
Nitpick comments:
In @.github/workflows/ci-packages.yml:
- Around line 74-79: The outputs block defines keys with hyphens (generation,
ios-build, android-build, ios-e2e, android-e2e) but the Node script and later
references use underscores (ios_build, android_build, ios_e2e, android_e2e);
rename the output keys to use underscores (e.g., ios_build, android_build,
ios_e2e, android_e2e) and update all references that read from the step outputs
(e.g., change needs.define-matrix.outputs.ios-build to
needs.define-matrix.outputs.ios_build and similarly for android and e2e keys,
and ensure any usage of set-matrix.outputs matches the new underscore names) so
naming is consistent across the outputs and their consumers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a43c2af-e87c-46b0-8a37-9dd796ca27bf
📒 Files selected for processing (3)
.github/actions/ios-build-xcode/action.yml.github/workflows/ci-packages.ymlscripts/e2e-maestro.sh
Comment on lines
+60
to
+65
| cd "$EXAMPLE_DIR/ios" | ||
|
|
||
| WORKSPACE_NAME="$(find . -maxdepth 1 -name "*.xcworkspace" ! -name "Pods.xcworkspace" -exec basename {} .xcworkspace \; | head -1)" | ||
| if [ -n "$WORKSPACE_NAME" ]; then | ||
| SCHEME="$WORKSPACE_NAME" | ||
| fi |
There was a problem hiding this comment.
Add failure handling for cd command.
If the directory doesn't exist or cd fails for any reason, the script will continue in the wrong directory and potentially cause confusing failures. Static analysis (Shellcheck SC2164) also flags this.
🛡️ Proposed fix
if [ "$PLATFORM" == "ios" ]; then
- cd "$EXAMPLE_DIR/ios"
+ cd "$EXAMPLE_DIR/ios" || { echo "Failed to change to iOS directory"; exit 1; }
WORKSPACE_NAME="$(find . -maxdepth 1 -name "*.xcworkspace" ! -name "Pods.xcworkspace" -exec basename {} .xcworkspace \; | head -1)"
if [ -n "$WORKSPACE_NAME" ]; then
SCHEME="$WORKSPACE_NAME"
fi📝 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.
Suggested change
| cd "$EXAMPLE_DIR/ios" | |
| WORKSPACE_NAME="$(find . -maxdepth 1 -name "*.xcworkspace" ! -name "Pods.xcworkspace" -exec basename {} .xcworkspace \; | head -1)" | |
| if [ -n "$WORKSPACE_NAME" ]; then | |
| SCHEME="$WORKSPACE_NAME" | |
| fi | |
| cd "$EXAMPLE_DIR/ios" || { echo "Failed to change to iOS directory"; exit 1; } | |
| WORKSPACE_NAME="$(find . -maxdepth 1 -name "*.xcworkspace" ! -name "Pods.xcworkspace" -exec basename {} .xcworkspace \; | head -1)" | |
| if [ -n "$WORKSPACE_NAME" ]; then | |
| SCHEME="$WORKSPACE_NAME" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 60-60: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/e2e-maestro.sh` around lines 60 - 65, The script does a cd to
"$EXAMPLE_DIR/ios" without checking for failure, so if the directory is missing
the rest of the logic (WORKSPACE_NAME/SCHEME) will run in the wrong path; update
the cd invocation (the line using cd "$EXAMPLE_DIR/ios") to check its exit
status and abort (e.g., cd ... || { echo "failed to cd to $EXAMPLE_DIR/ios" >&2;
exit 1; }) so the script stops on failure and avoids SC2164 warnings, leaving
the subsequent WORKSPACE_NAME and SCHEME logic unchanged.
| cd "$SCRIPT_DIR" | ||
| else | ||
| cd $EXAMPLE_DIR/android | ||
| cd "$EXAMPLE_DIR/android" |
There was a problem hiding this comment.
Add failure handling for cd command.
Same issue as the iOS path - the Android cd command should handle failures. Static analysis (Shellcheck SC2164) flags this as well.
🛡️ Proposed fix
else
- cd "$EXAMPLE_DIR/android"
+ cd "$EXAMPLE_DIR/android" || { echo "Failed to change to Android directory"; exit 1; }
chmod +x ./gradlew📝 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.
Suggested change
| cd "$EXAMPLE_DIR/android" | |
| else | |
| cd "$EXAMPLE_DIR/android" || { echo "Failed to change to Android directory"; exit 1; } | |
| chmod +x ./gradlew |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 134-134: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/e2e-maestro.sh` at line 134, The cd command that changes into the
Android example directory (the line containing cd "$EXAMPLE_DIR/android") must
handle failure; update that invocation to check its exit status and abort with a
descriptive error message if it fails (for example, append a fallback that logs
the failure and exits or test the return code and exit 1). Mirror the same
failure-handling approach used for the iOS cd path so the script does not
continue when the directory change fails.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci-packages.yml (2)
28-30:⚠️ Potential issue | 🟠 MajorReduce global workflow token privileges.
contents: writeat workflow scope is broader than required for this pipeline and increases blast radius. Prefer least-privilege defaults and elevate only where strictly needed.Suggested hardening
permissions: - contents: write - actions: write + contents: read + actions: write🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-packages.yml around lines 28 - 30, The workflow grants overly broad repository token scopes via the permissions block (permissions: contents: write and actions: write); change contents: write to contents: read to follow least-privilege, and remove or downgrade actions: write to the minimum required (or omit it at workflow scope and elevate to specific job-level permissions only where a job needs write access). Update the permissions declaration (referencing the permissions keys "contents" and "actions") so the default workflow token is read-only for contents and only grants write at the narrowest scope when strictly necessary.
220-246:⚠️ Potential issue | 🟠 MajorUse the same Yarn setup path in generation as in build/test jobs.
The
generate-packagesjob gates Node setup for Yarn (line 220) but omits the shared.github/actions/setup-yarnaction used in other jobs. Whenmatrix.pm == 'yarn', this skips Yarn v4.6.0 configuration, causing version drift between generate and test phases.Suggested fix
- name: Setup Node.js if: matrix.pm == 'yarn' uses: actions/setup-node@v6 with: node-version: 22.x + - name: Setup Yarn + if: matrix.pm == 'yarn' + uses: ./.github/actions/setup-yarn + with: + working-directory: ${{ github.workspace }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-packages.yml around lines 220 - 246, The generate-packages job currently sets up Node.js when matrix.pm == 'yarn' but omits the shared setup action used elsewhere; update the workflow so that the conditional Yarn branch uses the same .github/actions/setup-yarn action (the same invocation used in build/test jobs) before or instead of the Setup Node.js step to ensure Yarn v4.6.0 is installed and configured; locate the conditional step labeled "Setup Node.js" (and the subsequent "Setup Bun.js"/"Install repository dependencies"/"Build CLI and Link Locally" sequence) and replace or augment it to call the shared setup-yarn action when matrix.pm == 'yarn', preserving existing node-version and any matrix conditions.
🧹 Nitpick comments (1)
scripts/e2e-maestro.sh (1)
37-39: Consider validating EXAMPLE_DIR before deriving identifiers.If
EXAMPLE_DIRis empty or invalid, thecdin the subshell fails silently, causingPACKAGE_ROOT_NAMEto become.(current directory name), which would produce incorrectPACKAGE_NAMEandAPP_IDvalues. This could lead to confusing failures later in the script.🛡️ Proposed fix to validate EXAMPLE_DIR early
EXAMPLE_DIR=${2:-} PACKAGE_TYPE=${3:-} +if [ -z "$EXAMPLE_DIR" ] || [ ! -d "$EXAMPLE_DIR" ]; then + echo "Error! EXAMPLE_DIR is not set or does not exist: $EXAMPLE_DIR" + exit 1 +fi + echo "🚀 Running e2e video recording for $PLATFORM"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/e2e-maestro.sh` around lines 37 - 39, Validate EXAMPLE_DIR before using it: check that EXAMPLE_DIR is non-empty and points to an existing directory (e.g., test -n "$EXAMPLE_DIR" && [ -d "$EXAMPLE_DIR" ]), and if not, print a clear error and exit early; then derive PACKAGE_ROOT_NAME, PACKAGE_NAME, and APP_ID only after the validation succeeds so the subshell cd and basename calls cannot silently return "." and produce incorrect identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/ci-packages.yml:
- Around line 28-30: The workflow grants overly broad repository token scopes
via the permissions block (permissions: contents: write and actions: write);
change contents: write to contents: read to follow least-privilege, and remove
or downgrade actions: write to the minimum required (or omit it at workflow
scope and elevate to specific job-level permissions only where a job needs write
access). Update the permissions declaration (referencing the permissions keys
"contents" and "actions") so the default workflow token is read-only for
contents and only grants write at the narrowest scope when strictly necessary.
- Around line 220-246: The generate-packages job currently sets up Node.js when
matrix.pm == 'yarn' but omits the shared setup action used elsewhere; update the
workflow so that the conditional Yarn branch uses the same
.github/actions/setup-yarn action (the same invocation used in build/test jobs)
before or instead of the Setup Node.js step to ensure Yarn v4.6.0 is installed
and configured; locate the conditional step labeled "Setup Node.js" (and the
subsequent "Setup Bun.js"/"Install repository dependencies"/"Build CLI and Link
Locally" sequence) and replace or augment it to call the shared setup-yarn
action when matrix.pm == 'yarn', preserving existing node-version and any matrix
conditions.
---
Nitpick comments:
In `@scripts/e2e-maestro.sh`:
- Around line 37-39: Validate EXAMPLE_DIR before using it: check that
EXAMPLE_DIR is non-empty and points to an existing directory (e.g., test -n
"$EXAMPLE_DIR" && [ -d "$EXAMPLE_DIR" ]), and if not, print a clear error and
exit early; then derive PACKAGE_ROOT_NAME, PACKAGE_NAME, and APP_ID only after
the validation succeeds so the subshell cd and basename calls cannot silently
return "." and produce incorrect identifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3bc4cad9-aff3-457e-bd6c-ab6c0dc6e2ad
📒 Files selected for processing (3)
.github/workflows/ci-packages.ymle2e-tests/view.e2e.yamlscripts/e2e-maestro.sh
✅ Files skipped from review due to trivial changes (1)
- e2e-tests/view.e2e.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation