Feat/react native harness support#475
Open
patrickkabwe wants to merge 9 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional React Native Harness generation path (CLI flag ChangesReact Native Harness and package generation
sequenceDiagram
participant CLI as CLI (create)
participant Factory as NitroModuleFactory
participant FS as Filesystem (example, workflows)
participant CI as GitHub Actions
CLI->>Factory: call create with includeHarness, monorepo, packagePath
Factory->>FS: write example app, harness config, jest, tests, scripts
Factory->>FS: emit/transform workflows (harness-<platform>.yml), workspace files
CI->>FS: pick up generated workflows/artifacts
CI->>Factory: run harness jobs -> execute bun run test:harness:<platform>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-local.sh`:
- Around line 72-73: The expect script currently sends "n\r" to decline the
React Native harness prompt; change the send to just "\r" to accept the default
(consistent with other prompts) for the prompt matching "Include React Native
Harness for native Android and iOS tests?" in test-local.sh; this aligns with
the CLI's initialValue: false setting in src/cli/create.ts and makes the test
pattern consistent with the other default prompts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05f91da0-d51d-4324-8eaf-3b01415a827b
📒 Files selected for processing (13)
README.mddocs/docs/commands.mddocs/docs/usage/create-a-nitro-module.mdeslint.config.jssrc/cli/create.tssrc/cli/index.tssrc/code-snippets/code.js.tssrc/constants.tssrc/file-generators/cpp-file-generator.tssrc/generate-nitro-package.tssrc/types.tssrc/utils.tstest-local.sh
💤 Files with no reviewable changes (2)
- src/file-generators/cpp-file-generator.ts
- src/utils.ts
Comment on lines
+72
to
+73
| # React Native Harness (Default to no) | ||
| expect "Include React Native Harness for native Android and iOS tests?" {send "n\r"} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check `@clack/prompts` documentation for confirm prompt input handling
rg -A 10 'initialValue.*false' --type ts -g '**/src/cli/create.ts' | head -20Repository: patrickkabwe/create-nitro-module
Length of output: 821
🏁 Script executed:
# Find and read the test-local.sh file around lines 72-73
fd -t f test-local.shRepository: patrickkabwe/create-nitro-module
Length of output: 90
🏁 Script executed:
# Search for the React Native Harness prompt in create.ts
rg 'React Native Harness' --type ts -B 5 -A 5Repository: patrickkabwe/create-nitro-module
Length of output: 4981
🏁 Script executed:
# Check for other expect patterns in test files to compare patterns
rg 'expect.*send' -g '*.sh' -B 1 -A 1 | head -50Repository: patrickkabwe/create-nitro-module
Length of output: 730
Use "\r" to accept the default "no" response, consistent with other default prompts in the test file.
The prompt at line 72-73 sends "n\r" to decline the harness option. However, the test file uses "\r" alone to accept defaults for module type (line 74) and package manager (line 77). Since the harness prompt specifies initialValue: false in src/cli/create.ts line 531, sending just "\r" would be more consistent with the established pattern and less fragile:
expect "Include React Native Harness for native Android and iOS tests?" {send \r}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-local.sh` around lines 72 - 73, The expect script currently sends "n\r"
to decline the React Native harness prompt; change the send to just "\r" to
accept the default (consistent with other prompts) for the prompt matching
"Include React Native Harness for native Android and iOS tests?" in
test-local.sh; this aligns with the CLI's initialValue: false setting in
src/cli/create.ts and makes the test pattern consistent with the other default
prompts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/e2e-maestro.sh`:
- Around line 67-69: The variable holding the simulator UUID is assigned to
iphone16Id but later checks use iphone17Id, causing the boot-state logic to
reference the wrong variable; update the boot-state check (and any other
occurrences) to use the same identifier you assigned (either change the
assignment to iphone17Id or replace all iphone17Id references with iphone16Id)
so the echo, boot check and subsequent boot logic consistently reference the
same variable (iphone16Id/iphone17Id) throughout the script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a02fa0a9-b84d-485b-9a31-e2b6ca838ee7
📒 Files selected for processing (7)
.github/actions/ios-build-xcode/action.ymlassets/template/.github/workflows/ios-build.ymlassets/template/gitignorescripts/e2e-maestro.shsrc/code-snippets/code.js.tssrc/generate-nitro-package.tstest-local.sh
✅ Files skipped from review due to trivial changes (1)
- assets/template/gitignore
🚧 Files skipped from review as they are similar to previous changes (3)
- test-local.sh
- src/code-snippets/code.js.ts
- src/generate-nitro-package.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci-packages.yml:
- Around line 76-77: The workflow uses hyphenated output keys (e.g.,
ios-harness, android-harness) in job outputs and expression references which
breaks GitHub Actions dot-notation parsing; change all output key names to use
underscores to match the step outputs (ios_harness, android_harness) and update
every reference that reads them (for example any
needs.define-matrix.outputs.android-harness or .ios-harness at locations like
the ones mentioned) to the underscore form
(needs.define-matrix.outputs.android_harness and ios_harness), and ensure the
job output mappings that expose step outputs also use the underscore keys so all
producers and consumers use the same underscore-keyed names.
In `@src/generate-nitro-package.ts`:
- Around line 742-753: The current extraction uses the first match of
PRODUCT_BUNDLE_IDENTIFIER (bundleIdMatch) which can pick a Tests/UITests target;
update the logic in the function that reads projectFileContent to find all
PRODUCT_BUNDLE_IDENTIFIER occurrences and choose the one belonging to the main
app target: parse projectFileContent for target blocks (or scan with a global
regex), for each match examine surrounding context (e.g., the target name or the
block that contains the identifier) and skip targets whose names include
"Test"/"Tests"/"UITest"/"UITests" (or otherwise identify non-app targets), then
return the bundle id from the first remaining app target (trim and
replaceAll('"','') as before) and throw the existing error only if no suitable
app bundle id is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76ed2ba6-6469-4a5a-985e-0864e7937f1c
📒 Files selected for processing (7)
.github/workflows/ci-packages.ymlsrc/cli/create.tssrc/cli/index.tssrc/code-snippets/code.js.tssrc/constants.tssrc/generate-nitro-package.tssrc/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/code-snippets/code.js.ts
- src/constants.ts
- src/cli/create.ts
Comment on lines
+76
to
+77
| ios-harness: ${{ steps.set-matrix.outputs.ios_harness }} | ||
| android-harness: ${{ steps.set-matrix.outputs.android_harness }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify hyphenated output keys and dot-notation usages in this workflow.
rg -n "ios-harness|android-harness|outputs\.(ios|android)-harness" .github/workflows/ci-packages.ymlRepository: patrickkabwe/create-nitro-module
Length of output: 394
🏁 Script executed:
cat -n .github/workflows/ci-packages.yml | sed -n '50,100p'Repository: patrickkabwe/create-nitro-module
Length of output: 2224
🏁 Script executed:
cat -n .github/workflows/ci-packages.yml | sed -n '79,130p'Repository: patrickkabwe/create-nitro-module
Length of output: 2400
🏁 Script executed:
cat -n .github/workflows/ci-packages.yml | sed -n '130,250p'Repository: patrickkabwe/create-nitro-module
Length of output: 5507
🏁 Script executed:
cat -n .github/workflows/ci-packages.yml | sed -n '330,345p'Repository: patrickkabwe/create-nitro-module
Length of output: 776
🏁 Script executed:
cat -n .github/workflows/ci-packages.yml | sed -n '414,430p'Repository: patrickkabwe/create-nitro-module
Length of output: 778
Use underscore-keyed outputs consistently; dot notation with hyphens breaks in GitHub Actions expressions
The step set-matrix produces outputs with underscores (ios_harness, android_harness), but job outputs expose them with hyphens. Lines 337 and 421 then attempt to access these hyphenated keys using dot notation (needs.define-matrix.outputs.android-harness), which fails because GitHub Actions expression parsing cannot handle hyphens in dot notation. Rename all output keys to use underscores throughout for consistency.
Suggested fix
- ios-harness: ${{ steps.set-matrix.outputs.ios_harness }}
- android-harness: ${{ steps.set-matrix.outputs.android_harness }}
+ ios_harness: ${{ steps.set-matrix.outputs.ios_harness }}
+ android_harness: ${{ steps.set-matrix.outputs.android_harness }}
...
- matrix: ${{ fromJson(needs.define-matrix.outputs.android-harness) }}
+ matrix: ${{ fromJson(needs.define-matrix.outputs.android_harness) }}
...
- matrix: ${{ fromJson(needs.define-matrix.outputs.ios-harness) }}
+ matrix: ${{ fromJson(needs.define-matrix.outputs.ios_harness) }}Also applies to: 337, 421
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci-packages.yml around lines 76 - 77, The workflow uses
hyphenated output keys (e.g., ios-harness, android-harness) in job outputs and
expression references which breaks GitHub Actions dot-notation parsing; change
all output key names to use underscores to match the step outputs (ios_harness,
android_harness) and update every reference that reads them (for example any
needs.define-matrix.outputs.android-harness or .ios-harness at locations like
the ones mentioned) to the underscore form
(needs.define-matrix.outputs.android_harness and ios_harness), and ensure the
job output mappings that expose step outputs also use the underscore keys so all
producers and consumers use the same underscore-keyed names.
Comment on lines
+742
to
+753
| const bundleIdMatch = projectFileContent.match( | ||
| /PRODUCT_BUNDLE_IDENTIFIER = ([^;]+);/ | ||
| ) | ||
|
|
||
| if (bundleIdMatch == null) { | ||
| throw new Error( | ||
| `Failed to resolve iOS bundle identifier for React Native Harness from ${projectFilePath}` | ||
| ) | ||
| } | ||
|
|
||
| return bundleIdMatch[1].trim().replaceAll('"', '') | ||
| } |
There was a problem hiding this comment.
Bundle ID extraction can target the wrong iOS target
On Line 742, taking the first PRODUCT_BUNDLE_IDENTIFIER match is unsafe in multi-target projects (Tests/UITests can appear first), which can break generated harness config.
Suggested fix
- const bundleIdMatch = projectFileContent.match(
- /PRODUCT_BUNDLE_IDENTIFIER = ([^;]+);/
- )
-
- if (bundleIdMatch == null) {
+ const bundleIds = Array.from(
+ projectFileContent.matchAll(/PRODUCT_BUNDLE_IDENTIFIER = ([^;]+);/g),
+ match => match[1].trim().replaceAll('"', '')
+ )
+ const bundleId = bundleIds.find(
+ id => !id.endsWith('Tests') && !id.endsWith('UITests')
+ )
+
+ if (bundleId == null) {
throw new Error(
`Failed to resolve iOS bundle identifier for React Native Harness from ${projectFilePath}`
)
}
-
- return bundleIdMatch[1].trim().replaceAll('"', '')
+ return bundleId🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/generate-nitro-package.ts` around lines 742 - 753, The current extraction
uses the first match of PRODUCT_BUNDLE_IDENTIFIER (bundleIdMatch) which can pick
a Tests/UITests target; update the logic in the function that reads
projectFileContent to find all PRODUCT_BUNDLE_IDENTIFIER occurrences and choose
the one belonging to the main app target: parse projectFileContent for target
blocks (or scan with a global regex), for each match examine surrounding context
(e.g., the target name or the block that contains the identifier) and skip
targets whose names include "Test"/"Tests"/"UITest"/"UITests" (or otherwise
identify non-app targets), then return the bundle id from the first remaining
app target (trim and replaceAll('"','') as before) and throw the existing error
only if no suitable app bundle id is found.
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)
src/generate-nitro-package.ts (1)
683-697:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse monorepo-aware
node_modulespaths inbuild.gradle.These replacements are correct for the single-package layout, but in monorepo mode
example/android/app/build.gradlesits underpackages/<name>/....../../../node_modulesand$rootDir/../../node_modulesthen resolve inside the package workspace, not the monorepo root, so Android builds/codegen won't find React Native, Codegen, or Hermes.Suggested fix
- const gradleReplacements = { + const appNodeModulesPath = this.config.monorepo + ? '../../../../../node_modules' + : '../../../node_modules' + const androidNodeModulesPath = this.config.monorepo + ? '../../../../node_modules' + : '../../node_modules' + + const gradleReplacements = { '// reactNativeDir = file("../../node_modules/react-native")': - 'reactNativeDir = file("../../../node_modules/react-native")', + `reactNativeDir = file("${appNodeModulesPath}/react-native")`, '// codegenDir = file("../../node_modules/@react-native/codegen")': - 'codegenDir = file("../../../node_modules/@react-native/codegen")', + `codegenDir = file("${appNodeModulesPath}/@react-native/codegen")`, '// cliFile = file("../../node_modules/react-native/cli.js")': - 'cliFile = file("../../../node_modules/react-native/cli.js")', + `cliFile = file("${appNodeModulesPath}/react-native/cli.js")`, '// hermesCommand = "$rootDir/my-custom-hermesc/bin/hermesc"': - 'hermesCommand = "$rootDir/../../node_modules/hermes-compiler/hermesc/%OS-BIN%/hermesc"', + `hermesCommand = "$rootDir/${androidNodeModulesPath}/hermes-compiler/hermesc/%OS-BIN%/hermesc"`, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/generate-nitro-package.ts` around lines 683 - 697, The gradleReplacements object currently inserts paths that only work for single-package layouts; update the replacements so they are monorepo-aware by using one more ../ level: change the three-parent paths to four-parent paths (use 'file("../../../../node_modules/react-native")', 'file("../../../../node_modules/@react-native/codegen")', 'file("../../../../node_modules/react-native/cli.js")') and change the hermesCommand rootDir replacement to '$rootDir/../../../node_modules/hermes-compiler/hermesc/%OS-BIN%/hermesc' so that replacePlaceholder (called when creating androidBuildGradleData) will point at the monorepo root node_modules when example/android/app is under packages/<name>/...
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/generate-nitro-package.ts`:
- Around line 541-554: The harness version probing still calls getLatestVersion
even when skip-install is requested; update the includeHarness branch in
generate-nitro-package.ts to check the skipInstall option (e.g.
this.options.skipInstall or equivalent) before invoking getLatestVersion and, if
skipInstall is true, short-circuit the Promise.all to return null/undefined
versions for react-native-harness, `@react-native-harness/platform-android`,
`@react-native-harness/platform-apple` and the optional `@react-native-harness/ui`
(based on this.config.packageType === Nitro.View) so no npm/pnpm view network
calls are made; modify the code around this.config.includeHarness and the
Promise.all that calls getLatestVersion to respect skipInstall.
---
Outside diff comments:
In `@src/generate-nitro-package.ts`:
- Around line 683-697: The gradleReplacements object currently inserts paths
that only work for single-package layouts; update the replacements so they are
monorepo-aware by using one more ../ level: change the three-parent paths to
four-parent paths (use 'file("../../../../node_modules/react-native")',
'file("../../../../node_modules/@react-native/codegen")',
'file("../../../../node_modules/react-native/cli.js")') and change the
hermesCommand rootDir replacement to
'$rootDir/../../../node_modules/hermes-compiler/hermesc/%OS-BIN%/hermesc' so
that replacePlaceholder (called when creating androidBuildGradleData) will point
at the monorepo root node_modules when example/android/app is under
packages/<name>/...
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36c6992e-111e-4b46-a86b-dd297d863d4b
📒 Files selected for processing (3)
.github/actions/ios-build-xcode/action.ymlsrc/code-snippets/code.js.tssrc/generate-nitro-package.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/actions/ios-build-xcode/action.yml
- src/code-snippets/code.js.ts
Comment on lines
+541
to
+554
| if (this.config.includeHarness) { | ||
| const [ | ||
| reactNativeHarnessVersion, | ||
| androidHarnessVersion, | ||
| appleHarnessVersion, | ||
| uiHarnessVersion, | ||
| ] = await Promise.all([ | ||
| this.getLatestVersion('react-native-harness'), | ||
| this.getLatestVersion('@react-native-harness/platform-android'), | ||
| this.getLatestVersion('@react-native-harness/platform-apple'), | ||
| this.config.packageType === Nitro.View | ||
| ? this.getLatestVersion('@react-native-harness/ui') | ||
| : Promise.resolve(null), | ||
| ]) |
There was a problem hiding this comment.
Honor skipInstall before probing harness package versions.
This branch still shells out to npm/pnpm view even when --skip-install is set. That makes the skip-install path depend on registry availability and slows down/offlines the exact scenario this flag is meant to support.
Suggested fix
if (this.config.includeHarness) {
- const [
- reactNativeHarnessVersion,
- androidHarnessVersion,
- appleHarnessVersion,
- uiHarnessVersion,
- ] = await Promise.all([
- this.getLatestVersion('react-native-harness'),
- this.getLatestVersion('@react-native-harness/platform-android'),
- this.getLatestVersion('@react-native-harness/platform-apple'),
- this.config.packageType === Nitro.View
- ? this.getLatestVersion('@react-native-harness/ui')
- : Promise.resolve(null),
- ])
+ const [
+ reactNativeHarnessVersion,
+ androidHarnessVersion,
+ appleHarnessVersion,
+ uiHarnessVersion,
+ ] = this.config.skipInstall
+ ? [null, null, null, null]
+ : await Promise.all([
+ this.getLatestVersion('react-native-harness'),
+ this.getLatestVersion(
+ '@react-native-harness/platform-android'
+ ),
+ this.getLatestVersion(
+ '@react-native-harness/platform-apple'
+ ),
+ this.config.packageType === Nitro.View
+ ? this.getLatestVersion('@react-native-harness/ui')
+ : Promise.resolve(null),
+ ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/generate-nitro-package.ts` around lines 541 - 554, The harness version
probing still calls getLatestVersion even when skip-install is requested; update
the includeHarness branch in generate-nitro-package.ts to check the skipInstall
option (e.g. this.options.skipInstall or equivalent) before invoking
getLatestVersion and, if skipInstall is true, short-circuit the Promise.all to
return null/undefined versions for react-native-harness,
`@react-native-harness/platform-android`, `@react-native-harness/platform-apple` and
the optional `@react-native-harness/ui` (based on this.config.packageType ===
Nitro.View) so no npm/pnpm view network calls are made; modify the code around
this.config.includeHarness and the Promise.all that calls getLatestVersion to
respect skipInstall.
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
Documentation
Chores / CI & Tests
Repository