Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughRe-enables the BrowserStack Android CI job; cleans and documents the BrowserStack E2E workflow; adds a post-test step to extract Detox session_id and query BrowserStack; updates Detox runner args; and applies Android packaging, settings, and npm script changes. ChangesBrowserStack ID-based app resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
|
tsdamas
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
PingTestRunner/package.json (1)
24-24: 💤 Low valueConsider aligning Gradle task qualification across build scripts.
The
build:bs:androidscript now uses fully-qualified task names (:app:assembleRelease :app:assembleAndroidTest), which is more explicit and safer in multi-project builds. However, the.detoxrc.jsfile'sandroid.releasebuild command (line 40) still uses unqualified task names (assembleRelease assembleAndroidTest).While these serve different code paths (BrowserStack vs. local), the inconsistency could create maintenance confusion.
🤖 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 `@PingTestRunner/package.json` at line 24, The android release build command is inconsistent: package.json's "build:bs:android" uses fully-qualified Gradle tasks while .detoxrc.js's android.release uses unqualified task names; update the android.release build command in .detoxrc.js to use the fully-qualified tasks (e.g., replace "assembleRelease assembleAndroidTest" with ":app:assembleRelease :app:assembleAndroidTest") so both build scripts use the same explicit task qualification.
🤖 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.
Nitpick comments:
In `@PingTestRunner/package.json`:
- Line 24: The android release build command is inconsistent: package.json's
"build:bs:android" uses fully-qualified Gradle tasks while .detoxrc.js's
android.release uses unqualified task names; update the android.release build
command in .detoxrc.js to use the fully-qualified tasks (e.g., replace
"assembleRelease assembleAndroidTest" with ":app:assembleRelease
:app:assembleAndroidTest") so both build scripts use the same explicit task
qualification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a4487e1-002e-415b-9af7-d883813c10d9
⛔ Files ignored due to path filters (2)
.yarn/install-state.gzis excluded by!**/.yarn/**,!**/*.gzyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
.github/workflows/browserstack-e2e-android.ymlPingTestRunner/android/app/build.gradlePingTestRunner/android/settings.gradlePingTestRunner/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/browserstack-e2e-android.yml
Contributor
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/browserstack-e2e-android.yml:
- Around line 207-223: Change the BrowserStack session fetch in the View
BrowserStack test results step to call the correct GET endpoint and surface curl
failures: replace the POST to
"https://api-cloud.browserstack.com/app-automate/detox/v2/android/sessions/${session_id}"
with a GET to
"https://api-cloud.browserstack.com/app-automate/detox/v2/android/sessions/${session_id}.json",
run curl so it fails on non-2xx responses (check exit code / use --fail) and
print a clear warning including the session_id on error; optionally pipe the
response through jq to pretty-print successful JSON. Ensure you update the curl
invocation that references session_id and the step that echoes "Fetching
BrowserStack session details for session: $session_id".
In `@PingTestRunner/android/app/src/main/res/xml/network_security_config.xml`:
- Around line 15-27: The network_security_config.xml in src/main currently
enables cleartext globally via the <base-config cleartextTrafficPermitted="true"
/> referenced by android:networkSecurityConfig and combined with
android:usesCleartextTraffic="true", which makes cleartext available to all
builds; move that permissive base-config into a variant-scoped overlay (e.g.,
src/debug/res/xml/network_security_config.xml or
src/androidTest/res/xml/network_security_config.xml) and remove or replace the
permissive <base-config> in src/main/res/xml/network_security_config.xml with a
restrictive policy, then update the debug/androidTest manifest overlay (or
manifestPlaceholders) to reference the permissive resource (and/or set
android:usesCleartextTraffic="true" only in debug/androidTest) so only
Detox/Test runs (DetoxTest.kt targeting ws://10.0.2.2) allow cleartext.
🪄 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: bd8907c4-5f00-42e9-a015-f809e975a496
📒 Files selected for processing (3)
.github/workflows/browserstack-e2e-android.ymlPingTestRunner/.detoxrc.jsPingTestRunner/android/app/src/main/res/xml/network_security_config.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- PingTestRunner/.detoxrc.js
Comment on lines
+15
to
+27
| <!-- | ||
| base-config (not domain-config) is intentional: Android's NetworkSecurityTrustManager | ||
| enforces the 3-arg checkServerTrusted(certs, authType, hostname) for ALL HTTPS connections | ||
| whenever any <domain-config> block is present in this file. The Ping network SDK uses | ||
| Ktor CIO which calls the 2-arg variant only, causing every server call to throw: | ||
| "Domain specific configurations require that hostname aware | ||
| checkServerTrusted(X509Certificate[], String, String) is used" | ||
| Switching to <base-config> removes the domain-specific enforcement trigger while still | ||
| permitting cleartext traffic for Detox instrumentation (localhost, emulator host). | ||
| The AndroidManifest.xml android:usesCleartextTraffic="true" attribute provides an | ||
| additional belt-and-suspenders guarantee. | ||
| --> | ||
| <base-config cleartextTrafficPermitted="true" /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant files with line numbers (small targeted reads).
sed -n '1,120p' PingTestRunner/android/app/src/main/res/xml/network_security_config.xml | nl -ba
echo "---- AndroidManifest.xml (snippet) ----"
# Print a reasonable window around where networkSecurityConfig likely appears
sed -n '1,120p' PingTestRunner/android/app/src/main/AndroidManifest.xml | nl -ba | sed -n '1,120p'
echo "---- DetoxTest.kt (snippet) ----"
# Print DetoxTest around the referenced lines
sed -n '1,120p' PingTestRunner/android/app/src/androidTest/java/com/pingtestrunner/DetoxTest.kt | nl -ba
echo "---- Search for other network security config files/overlays ----"
# Find any other network_security_config.xml under the Android module
fd -t f network_security_config.xml PingTestRunner/android/app/src -a | sort
# Also search for base-config or cleartextTrafficPermitted occurrences anywhere in Android resources.
rg -n "cleartextTrafficPermitted|networkSecurityConfig|base-config" PingTestRunner/android/app/src/main PingTestRunner/android/app/src/androidTest PingTestRunner/android/app/src/debug || true
echo "---- Check for debug overrides / resource overlays ----"
# Look for other XML files with network security config names or Android network security config refs.
rg -n "android:networkSecurityConfig|network_security_config" PingTestRunner/android/app/src || true
# Print gradle buildTypes/flavors if present to reason about debug overlays
echo "---- build.gradle files (relevant sections) ----"
for f in \
PingTestRunner/android/app/build.gradle \
PingTestRunner/android/app/build.gradle.kts \
PingTestRunner/android/build.gradle \
PingTestRunner/android/build.gradle.kts; do
if [ -f "$f" ]; then
echo "== $f =="
sed -n '1,200p' "$f" | nl -ba | head -n 200
fi
doneRepository: ForgeRock/ping-react-native-sdk
Length of output: 118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- network_security_config.xml (first 200 lines) ----"
sed -n '1,200p' PingTestRunner/android/app/src/main/res/xml/network_security_config.xml | cat -n
echo "---- AndroidManifest.xml (first 200 lines) ----"
sed -n '1,200p' PingTestRunner/android/app/src/main/AndroidManifest.xml | cat -n
echo "---- DetoxTest.kt (lines 1-140) ----"
sed -n '1,140p' PingTestRunner/android/app/src/androidTest/java/com/pingtestrunner/DetoxTest.kt | cat -n
echo "---- Find other network_security_config.xml ----"
fd -t f network_security_config.xml PingTestRunner/android/app/src -a 2>/dev/null | sort || true
echo "---- Search for networkSecurityConfig / cleartextTrafficPermitted usage ----"
rg -n "cleartextTrafficPermitted|networkSecurityConfig|network_security_config|base-config" PingTestRunner/android/app/src/main PingTestRunner/android/app/src/androidTest PingTestRunner/android/app/src/debug 2>/dev/null || true
echo "---- Search for debug resource overlays dirs ----"
fd -t d -a --max-depth 5 "debug" PingTestRunner/android/app/src 2>/dev/null | sort || true
echo "---- build gradle snippets (first 240 lines) ----"
for f in \
PingTestRunner/android/app/build.gradle \
PingTestRunner/android/app/build.gradle.kts \
PingTestRunner/android/build.gradle \
PingTestRunner/android/build.gradle.kts; do
if [ -f "$f" ]; then
echo "== $f =="
sed -n '1,240p' "$f" | cat -n | head -n 240
fi
doneRepository: ForgeRock/ping-react-native-sdk
Length of output: 13408
Scope cleartext override to Detox/test builds only
src/main/res/xml/network_security_config.xml sets <base-config cleartextTrafficPermitted="true" />, and the app references it from the main manifest (android:networkSecurityConfig="@xml/network_security_config") while also enabling android:usesCleartextTraffic="true". That makes cleartext permissive for all destinations, not just the Detox emulator-host WS endpoint (ws://10.0.2.2) rewritten in DetoxTest.kt. Move the permissive config to a debug/androidTest-scoped overlay (or equivalent variant-specific resource/manifest) and keep src/main restrictive.
🤖 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 `@PingTestRunner/android/app/src/main/res/xml/network_security_config.xml`
around lines 15 - 27, The network_security_config.xml in src/main currently
enables cleartext globally via the <base-config cleartextTrafficPermitted="true"
/> referenced by android:networkSecurityConfig and combined with
android:usesCleartextTraffic="true", which makes cleartext available to all
builds; move that permissive base-config into a variant-scoped overlay (e.g.,
src/debug/res/xml/network_security_config.xml or
src/androidTest/res/xml/network_security_config.xml) and remove or replace the
permissive <base-config> in src/main/res/xml/network_security_config.xml with a
restrictive policy, then update the debug/androidTest manifest overlay (or
manifestPlaceholders) to reference the permissive resource (and/or set
android:usesCleartextTraffic="true" only in debug/androidTest) so only
Detox/Test runs (DetoxTest.kt targeting ws://10.0.2.2) allow cleartext.
Contributor
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 `@PingTestRunner/.detoxrc.js`:
- Line 20: Remove the forced CI override of maxWorkers in the Detox config by
eliminating or conditionalizing the line setting maxWorkers: process.env.CI ? 2
: undefined; instead let Detox/Jest pick the appropriate worker count (or
explicitly set it to 1) so it does not conflict with testRunner.args.maxWorkers
and the e2e/jest.config.js maxWorkers: 1; update .detoxrc.js to either omit
maxWorkers entirely on CI or set it to 1 to ensure only one worker/device
session is used.
🪄 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: 9827f892-46e9-444d-9f4a-3137e5fb0899
📒 Files selected for processing (1)
PingTestRunner/.detoxrc.js
…e workflow comment
…ey workflows Extract APK build into a dedicated prep workflow that uploads artifacts, then run core and journey E2E test suites concurrently on BrowserStack instead of serially in a single job.
…al vulnerabilities
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
Chores
New Features