chore: improvement to end to end test#7008
Conversation
WalkthroughAdds API request retry wrappers, centralizes keyboard dismissal into a shared Maestro flow and updates many tests to call it, enhances CI workflows (Maestro versioning, caching, emulator/simulator options, timeouts), and makes Android AVD creation dynamic and architecture-aware. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Rohit3523
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
app/views/ReportUserView/index.tsx (1)
90-90: Consider using'handled'instead of'always'for better UX balance.With
'always', users cannot dismiss the keyboard by tapping empty space—they must use the keyboard's done/return key or tap an interactive element. The'handled'option allows the submit button to receive taps without first dismissing the keyboard (which likely addresses the Maestro test needs), while still permitting keyboard dismissal when tapping non-interactive areas.- <ScrollView contentContainerStyle={styles.scroll} keyboardShouldPersistTaps='always'> + <ScrollView contentContainerStyle={styles.scroll} keyboardShouldPersistTaps='handled'>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/ReportUserView/index.tsx` at line 90, Change the ScrollView usage in ReportUserView to use keyboardShouldPersistTaps='handled' instead of 'always' to allow taps on non-interactive areas to dismiss the keyboard while still letting interactive elements (like the submit button) receive taps; locate the ScrollView component in ReportUserView (the element using contentContainerStyle={styles.scroll}) and update its keyboardShouldPersistTaps prop accordingly so UX and Maestro tests behave as expected..maestro/tests/teams/utils/create-channel.yaml (1)
37-37: Inconsistent quoting style with otherrunFlowreferences.Line 37 uses a quoted path (
'../../../helpers/hide-keyboard.yaml'), while line 61 uses an unquoted path (../../../helpers/go-back.yaml). Consider removing the quotes for consistency within the file.♻️ Suggested fix
- runFlow: '../../../helpers/hide-keyboard.yaml' + runFlow: ../../../helpers/hide-keyboard.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.maestro/tests/teams/utils/create-channel.yaml at line 37, The runFlow reference on line using 'runFlow' with '../../../helpers/hide-keyboard.yaml' is quoted while other entries like runFlow ../../../helpers/go-back.yaml are unquoted; update the runFlow entry that references hide-keyboard.yaml to use the same unquoted style (remove the surrounding quotes) so all runFlow paths (e.g., the hide-keyboard.yaml and go-back.yaml references) use a consistent quoting convention..maestro/scripts/data-setup.js (2)
73-73: Remove misleadingasynckeyword.The function is marked
asyncbut contains noawaitstatements. The retry wrappers (getWithRetry,postWithRetry) are synchronous. Removeasyncto avoid confusion.Suggested fix
-const deleteCreatedUser = async ({ username: usernameToDelete }) => { +const deleteCreatedUser = ({ username: usernameToDelete }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.maestro/scripts/data-setup.js at line 73, The function deleteCreatedUser is declared async but contains no await usage (it calls synchronous retry wrappers getWithRetry/postWithRetry), so remove the misleading async keyword from the deleteCreatedUser declaration to make it a plain function; update the function signature "deleteCreatedUser" accordingly and ensure any callers do not rely on it returning a Promise (if they do, wrap the call where necessary instead of keeping async here).
212-215: Consider adding a comment explaining the busy-wait approach.This busy-wait implementation blocks the thread. While this is likely intentional for Maestro's synchronous JS environment (where
setTimeoutcallbacks may not be available), a brief comment would help future maintainers understand the design choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.maestro/scripts/data-setup.js around lines 212 - 215, The busy-wait loop in function sleep(ms) blocks the thread intentionally; add a concise comment above the sleep function explaining that this synchronous busy-wait is deliberate because Maestro runs in a synchronous JS environment where setTimeout/async callbacks are not available (or unreliable), noting the tradeoff (CPU spin) and that it's used to pause execution deterministically; reference the function name sleep and keep the comment short and clear for future maintainers..github/workflows/maestro-android.yml (1)
25-31: Consider including more configuration in the cache key.The cache key only includes the API level, but the AVD configuration also depends on
arch,profile,disk-size, andram-size. If any of these change, the cache won't invalidate and a stale AVD could be used.♻️ Suggested improvement
- name: Cache Android AVD uses: actions/cache@v4 with: path: | ~/.android/avd/* ~/.android/adb* - key: avd-${{ runner.os }}-api34 + key: avd-${{ runner.os }}-api34-x86_64-pixel7pro🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/maestro-android.yml around lines 25 - 31, Update the cache key used in the "Cache Android AVD" step (actions/cache@v4) so it includes the AVD configuration dimensions in addition to the API level; specifically incorporate variables for arch, profile, disk-size and ram-size (and keep runner.os/api) into the key (e.g., using the same workflow/matrix variables you use to create the AVD) so changes to arch/profile/disk-size/ram-size correctly invalidate the cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/maestro-android.yml:
- Around line 33-37: The cache key for the "Cache Maestro" step uses maestro-${{
runner.os }} which will reuse cached installs across different Maestro versions;
update the workflow to include the MAESTRO_VERSION in the key (e.g., maestro-${{
runner.os }}-${{ env.MAESTRO_VERSION }}) and set MAESTRO_VERSION as a
workflow-level env variable (or reference the existing definition) so the cache
invalidates when the Maestro version changes; modify the step that currently
uses actions/cache@v4 and the key: maestro-${{ runner.os }} to reference the
version variable instead.
- Around line 49-54: In the "Install Maestro" workflow step restore curl's fail
flag: update the command that currently uses curl -Ls
"https://get.maestro.mobile.dev" | bash to use curl -fsSL
"https://get.maestro.mobile.dev" | bash so curl exits non‑zero on HTTP errors
instead of piping error pages to bash; keep the existing echo
"$HOME/.maestro/bin" >> $GITHUB_PATH and the MAESTRO_VERSION env as-is.
In @.github/workflows/maestro-ios.yml:
- Line 40: The shell redirection uses an unquoted variable which can break on
paths with spaces; update the echo redirections to quote the target
variables—replace uses of >> $GITHUB_PATH and >> $GITHUB_ENV with >>
"$GITHUB_PATH" and >> "$GITHUB_ENV" (keep the echoed string quoted as well, e.g.
echo "$HOME/.maestro/bin" >> "$GITHUB_PATH") so both the redirection target and
echoed content are safely quoted.
- Around line 90-93: Change the job retry strategy so failures and timeouts both
trigger retries: update the retry configuration (the retry_on field used
alongside timeout_minutes: 30 and max_attempts: 2 in the job that runs the
./.github/scripts/run-maestro.sh ios ${inputs.shard} command) from retry_on:
timeout to retry_on: any so the step will retry on non-zero exits as well as
timeouts.
In @.maestro/tests/room/ignoreuser.yaml:
- Line 218: Two test actions currently wait for selector id
'report-user-view-submit' but then tap by text 'Report', which can be flaky;
change both tap calls that use text 'Report' to tap using the deterministic id
selector 'report-user-view-submit' (i.e., replace the tap-by-text for the report
action with a tap that targets selector id 'report-user-view-submit') so the
test consistently interacts with the intended submit button.
In `@scripts/create-avd.sh`:
- Around line 46-53: The script currently appends hardware config lines to
CONFIG which creates duplicate keys on re-runs; update the block that writes to
CONFIG (referencing the CONFIG variable and the hw.* keys) to for each key
(hw.lcd.density, hw.lcd.height, hw.lcd.width, hw.gpu.enabled) either replace an
existing line or add it if missing (use sed/awk/in-place editing) instead of
using ">>", ensuring idempotency so subsequent runs update the existing entries
rather than appending duplicates; keep the final echo "AVD created: $AVD_NAME
($ABI)" unchanged.
---
Nitpick comments:
In @.github/workflows/maestro-android.yml:
- Around line 25-31: Update the cache key used in the "Cache Android AVD" step
(actions/cache@v4) so it includes the AVD configuration dimensions in addition
to the API level; specifically incorporate variables for arch, profile,
disk-size and ram-size (and keep runner.os/api) into the key (e.g., using the
same workflow/matrix variables you use to create the AVD) so changes to
arch/profile/disk-size/ram-size correctly invalidate the cache.
In @.maestro/scripts/data-setup.js:
- Line 73: The function deleteCreatedUser is declared async but contains no
await usage (it calls synchronous retry wrappers getWithRetry/postWithRetry), so
remove the misleading async keyword from the deleteCreatedUser declaration to
make it a plain function; update the function signature "deleteCreatedUser"
accordingly and ensure any callers do not rely on it returning a Promise (if
they do, wrap the call where necessary instead of keeping async here).
- Around line 212-215: The busy-wait loop in function sleep(ms) blocks the
thread intentionally; add a concise comment above the sleep function explaining
that this synchronous busy-wait is deliberate because Maestro runs in a
synchronous JS environment where setTimeout/async callbacks are not available
(or unreliable), noting the tradeoff (CPU spin) and that it's used to pause
execution deterministically; reference the function name sleep and keep the
comment short and clear for future maintainers.
In @.maestro/tests/teams/utils/create-channel.yaml:
- Line 37: The runFlow reference on line using 'runFlow' with
'../../../helpers/hide-keyboard.yaml' is quoted while other entries like runFlow
../../../helpers/go-back.yaml are unquoted; update the runFlow entry that
references hide-keyboard.yaml to use the same unquoted style (remove the
surrounding quotes) so all runFlow paths (e.g., the hide-keyboard.yaml and
go-back.yaml references) use a consistent quoting convention.
In `@app/views/ReportUserView/index.tsx`:
- Line 90: Change the ScrollView usage in ReportUserView to use
keyboardShouldPersistTaps='handled' instead of 'always' to allow taps on
non-interactive areas to dismiss the keyboard while still letting interactive
elements (like the submit button) receive taps; locate the ScrollView component
in ReportUserView (the element using contentContainerStyle={styles.scroll}) and
update its keyboardShouldPersistTaps prop accordingly so UX and Maestro tests
behave as expected.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/maestro-android.yml.github/workflows/maestro-ios.yml.maestro/helpers/create-account.yaml.maestro/helpers/search-room.yaml.maestro/scripts/data-setup.js.maestro/tests/assorted/broadcast.yaml.maestro/tests/assorted/change-avatar.yaml.maestro/tests/e2ee/flows/create-e2ee-room.yaml.maestro/tests/e2ee/utils/change-e2ee-key.yaml.maestro/tests/onboarding/change-password.yaml.maestro/tests/onboarding/login/invalid-credentials.yaml.maestro/tests/onboarding/login/login.yaml.maestro/tests/onboarding/register/create-account.yaml.maestro/tests/room/create-room.yaml.maestro/tests/room/ignoreuser.yaml.maestro/tests/room/room.yaml.maestro/tests/room/share-message.yaml.maestro/tests/teams/team.yaml.maestro/tests/teams/utils/create-channel.yamlapp/views/ReportUserView/index.tsxscripts/create-avd.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Build Android / android-build
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
🪛 actionlint (1.7.11)
.github/workflows/maestro-ios.yml
[error] 36-36: shellcheck reported issue in this script: SC2086:info:4:30: Double quote to prevent globbing and word splitting
(shellcheck)
[error] 75-75: shellcheck reported issue in this script: SC2086:info:3:22: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (25)
.maestro/tests/onboarding/login/invalid-credentials.yaml (1)
20-20: Good helper extraction for keyboard dismissal.Line 20’s switch to the shared
hide-keyboard.yamlflow improves consistency and keeps this test easier to maintain..maestro/tests/onboarding/register/create-account.yaml (1)
31-31: Good refactor to shared keyboard helper flow.This keeps keyboard-dismiss logic consistent and reusable across Maestro tests.
.maestro/scripts/data-setup.js (1)
217-252: LGTM on the retry implementation.The retry logic with exponential backoff is well-structured: correctly handles 2xx success, immediately fails on 4xx client errors (non-retryable), and retries on 5xx/network errors. The backoff calculation and loop bounds are correct.
.github/workflows/maestro-android.yml (3)
22-23: LGTM!Removing quotes around
temurinand17is a valid YAML formatting preference with no semantic change.
62-65: LGTM!Disabling unnecessary services to free resources for the emulator is a good CI optimization. The
|| truegracefully handles cases where services don't exist.
70-85: LGTM!The emulator configuration is well-suited for CI:
- 60-minute timeout is reasonable for test execution
- 900-second boot timeout provides adequate buffer
- Emulator options appropriately disable unnecessary features (audio, camera, boot animation) while enabling hardware acceleration
scripts/create-avd.sh (4)
1-2: LGTM!Adding
set -eensures the script exits immediately on any command failure, which is good practice for CI scripts.
4-15: LGTM!The ANDROID_HOME initialization logic correctly handles the fallback chain: existing value → ANDROID_SDK_ROOT → default path. Debug output aids CI troubleshooting.
17-31: LGTM!Good architecture detection that handles both macOS (
arm64) and Linux (aarch64) naming conventions, with a sensible fallback tox86_64for CI runners.
33-44: LGTM!The SDK installation and AVD creation steps are correctly structured. Using
echo "no"to decline custom hardware profile ensures default settings are applied..maestro/tests/e2ee/utils/change-e2ee-key.yaml (1)
10-10: Good move to shared keyboard handling.Centralizing keyboard dismissal via helper flow improves consistency across E2EE flows.
.maestro/helpers/search-room.yaml (1)
8-9: Nice stabilization before interacting with search UI.Waiting for animation completion here should reduce timing-related flakes.
.maestro/tests/assorted/change-avatar.yaml (1)
176-176: Looks good—helper reuse is consistent here.The shared hide-keyboard flow keeps this step aligned with the rest of the test suite.
.maestro/tests/e2ee/flows/create-e2ee-room.yaml (1)
47-47: LGTM on keyboard handling extraction.This keeps room-creation flow behavior consistent with other Maestro tests.
.maestro/tests/assorted/broadcast.yaml (1)
68-68: Good consistency update.Using the shared keyboard helper here should make broadcast test behavior more predictable.
.maestro/tests/onboarding/login/login.yaml (1)
21-21: Clean refactor to shared helper.This keeps onboarding login keyboard handling uniform with other flows.
.maestro/tests/room/share-message.yaml (1)
71-71: Looks good—shared flow usage is appropriate.Centralized keyboard dismissal should help reduce drift between message-sharing scenarios.
.maestro/tests/room/room.yaml (1)
164-164: Great consistency pass across all keyboard-dismiss points.Applying the same helper in all these locations makes the room flow easier to maintain and less flaky.
Also applies to: 490-490, 513-513, 568-568
.maestro/tests/onboarding/change-password.yaml (1)
29-29: Good reuse of shared keyboard helper.Replacing inline keyboard dismissal with
runFlowhere keeps this test aligned with the centralized helper approach.Also applies to: 35-35
.maestro/helpers/create-account.yaml (1)
27-27: Nice consolidation of keyboard handling.Using
runFlow: './hide-keyboard.yaml'here keeps the helper usage consistent and reduces duplicated inline actions..maestro/tests/teams/team.yaml (1)
109-109: Consistent helper usage across team flows.These replacements keep keyboard dismissal behavior centralized while preserving surrounding assertions and navigation steps.
Also applies to: 300-300, 319-319
.maestro/tests/room/create-room.yaml (1)
174-174: Good standardization across create-room scenarios.Centralizing keyboard hide behavior in all these branches should reduce duplication and keep maintenance easier.
Also applies to: 193-193, 261-261, 325-325, 395-395, 432-432
.maestro/tests/room/ignoreuser.yaml (2)
91-104: Cross-platform tap handling looks solid.The Android/iOS-specific tap branches are a good stabilization move for username header interactions.
Also applies to: 153-166, 232-247
212-212: Keyboard helper adoption is consistent here too.Using the shared hide-keyboard flow in both report paths improves consistency with the rest of the suite.
Also applies to: 269-269
.github/workflows/maestro-ios.yml (1)
61-68: Good simulator synchronization update.Using
simctl bootstatus -bplus warm-up delay is a solid stability improvement versus manual boot loops.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.maestro/scripts/data-setup.js (1)
200-206:⚠️ Potential issue | 🟠 MajorAsync function calls are not awaited, causing silent failures.
deleteCreatedUseris an async function (line 73), butdeleteCreatedUserscalls it in a synchronous loop withoutawait. This means:
- Errors from individual deletions won't propagate
- The function returns before deletions complete
- If multiple deletions run concurrently, they may interfere with shared
headersstate🐛 Proposed fix
// Delete created users to avoid use all the Seats Available on the server -const deleteCreatedUsers = () => { +const deleteCreatedUsers = async () => { if (data.accounts.length) { for (const deleteUser of data.accounts) { - deleteCreatedUser(deleteUser); + await deleteCreatedUser(deleteUser); } } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.maestro/scripts/data-setup.js around lines 200 - 206, The current deleteCreatedUsers function calls the async deleteCreatedUser without awaiting, causing silent failures and race conditions; change deleteCreatedUsers to be async and either (1) await each deleteCreatedUser call inside a for...of loop to run deletions sequentially and preserve shared headers/state, or (2) if you need concurrency, map data.accounts to promises and await Promise.all but ensure deleteCreatedUser does not rely on mutable shared headers (or pass isolated header copies) to avoid interference. Update any callers to await deleteCreatedUsers as needed.
♻️ Duplicate comments (2)
.github/workflows/maestro-android.yml (1)
53-53:⚠️ Potential issue | 🟡 MinorRe-add curl fail-fast for Maestro installer.
Line 53 uses
curl -Ls ... | bash; without-f, HTTP 4xx/5xx responses may be piped intobashinstead of failing early.🐛 Proposed fix
- curl -Ls "https://get.maestro.mobile.dev" | bash + curl -fLs "https://get.maestro.mobile.dev" | bash#!/bin/bash set -euo pipefail line="$(rg -n 'curl\s+-\S*\s+"https://get\.maestro\.mobile\.dev"' .github/workflows/maestro-android.yml)" echo "$line" # Verification condition: install command flags should include -f echo "$line" | grep -q -- '-f'Expected result: grep succeeds only when the curl flags include
-f.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/maestro-android.yml at line 53, Update the installer invocation so curl fails fast on HTTP errors by adding the -f flag to the existing command `curl -Ls "https://get.maestro.mobile.dev" | bash` (make it `curl -Lsf "https://get.maestro.mobile.dev" | bash`), i.e., modify the occurrence of that command in the workflow to include `-f` (so the pipeline won't feed error pages into `bash`) and ensure any whitespace-preserving uses of the string match the new flags for verification..github/workflows/maestro-ios.yml (1)
86-86:⚠️ Potential issue | 🟡 MinorQuote the
GITHUB_ENVredirection target.Line 86 uses
>> $GITHUB_ENV; quote it to avoid shell word-splitting/globbing risk and to satisfy SC2086.🐛 Proposed fix
- echo "UDID=$UDID" >> $GITHUB_ENV + echo "UDID=$UDID" >> "$GITHUB_ENV"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/maestro-ios.yml at line 86, The shell redirection uses an unquoted variable in the command echo "UDID=$UDID" >> $GITHUB_ENV which can trigger word-splitting/globbing and SC2086; update the redirection target to use the quoted variable (i.e., quote $GITHUB_ENV) so the shell treats the environment file path as a single token and avoids unintended expansion.
🧹 Nitpick comments (2)
.maestro/scripts/data-setup.js (2)
212-215: Busy-wait sleep implementation blocks execution.This spin-lock approach blocks the JavaScript event loop. While this may be intentional for Maestro's synchronous execution model, be aware it prevents any other operations during the wait period.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.maestro/scripts/data-setup.js around lines 212 - 215, The current sleep function (function sleep(ms)) is a busy-wait that blocks the event loop; replace it with a non-blocking implementation that returns a Promise which resolves after ms using setTimeout so callers can await sleep(ms). Update all call sites if necessary to use await or handle the returned Promise, and remove the spin-loop to avoid blocking other asynchronous operations.
73-95: Unnecessaryasynckeyword on synchronous function.
deleteCreatedUseris markedasyncbut only calls synchronous functions (login,getWithRetry,postWithRetry). Theasynckeyword is misleading and can be removed for clarity.♻️ Proposed simplification
-const deleteCreatedUser = async ({ username: usernameToDelete }) => { +const deleteCreatedUser = ({ username: usernameToDelete }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.maestro/scripts/data-setup.js around lines 73 - 95, The function deleteCreatedUser is declared async but only calls synchronous functions (login, getWithRetry, postWithRetry); remove the async keyword from the deleteCreatedUser declaration so it becomes a plain synchronous function, and scan for any callers that might be awaiting deleteCreatedUser to update them to treat it as a non-promise-returning function if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/maestro-ios.yml:
- Line 47: The curl invocation piping to bash should use fail-fast HTTP
behavior; update the curl command string "curl -Ls
https://get.maestro.mobile.dev | bash" to include the -f flag (i.e., "curl -fLs
...") so HTTP errors cause curl to exit non‑zero and fail the workflow instead
of streaming error pages into bash.
In `@scripts/create-avd.sh`:
- Around line 39-44: The AVD creation step can fail if an AVD named by AVD_NAME
already exists; update scripts/create-avd.sh to make creation idempotent by
checking for and removing any existing AVD before calling avdmanager create avd:
use avdmanager list avd or avdmanager list to detect an existing AVD with name
AVD_NAME and, if found, run avdmanager delete avd -n "$AVD_NAME" (or equivalent)
to remove it, then proceed with the avdmanager create avd ... -n "$AVD_NAME" -d
"pixel_7_pro" --package "$IMAGE" command.
- Around line 35-37: Accept Android SDK licenses before attempting installs: run
the license acceptance command (yes | sdkmanager --licenses) prior to invoking
sdkmanager --install emulator and sdkmanager "$IMAGE" so CI/fresh environments
won't fail under set -e; reorder the calls so sdkmanager --licenses (using the
same sdkmanager binary) executes first and only then run sdkmanager --install
emulator and sdkmanager "$IMAGE".
---
Outside diff comments:
In @.maestro/scripts/data-setup.js:
- Around line 200-206: The current deleteCreatedUsers function calls the async
deleteCreatedUser without awaiting, causing silent failures and race conditions;
change deleteCreatedUsers to be async and either (1) await each
deleteCreatedUser call inside a for...of loop to run deletions sequentially and
preserve shared headers/state, or (2) if you need concurrency, map data.accounts
to promises and await Promise.all but ensure deleteCreatedUser does not rely on
mutable shared headers (or pass isolated header copies) to avoid interference.
Update any callers to await deleteCreatedUsers as needed.
---
Duplicate comments:
In @.github/workflows/maestro-android.yml:
- Line 53: Update the installer invocation so curl fails fast on HTTP errors by
adding the -f flag to the existing command `curl -Ls
"https://get.maestro.mobile.dev" | bash` (make it `curl -Lsf
"https://get.maestro.mobile.dev" | bash`), i.e., modify the occurrence of that
command in the workflow to include `-f` (so the pipeline won't feed error pages
into `bash`) and ensure any whitespace-preserving uses of the string match the
new flags for verification.
In @.github/workflows/maestro-ios.yml:
- Line 86: The shell redirection uses an unquoted variable in the command echo
"UDID=$UDID" >> $GITHUB_ENV which can trigger word-splitting/globbing and
SC2086; update the redirection target to use the quoted variable (i.e., quote
$GITHUB_ENV) so the shell treats the environment file path as a single token and
avoids unintended expansion.
---
Nitpick comments:
In @.maestro/scripts/data-setup.js:
- Around line 212-215: The current sleep function (function sleep(ms)) is a
busy-wait that blocks the event loop; replace it with a non-blocking
implementation that returns a Promise which resolves after ms using setTimeout
so callers can await sleep(ms). Update all call sites if necessary to use await
or handle the returned Promise, and remove the spin-loop to avoid blocking other
asynchronous operations.
- Around line 73-95: The function deleteCreatedUser is declared async but only
calls synchronous functions (login, getWithRetry, postWithRetry); remove the
async keyword from the deleteCreatedUser declaration so it becomes a plain
synchronous function, and scan for any callers that might be awaiting
deleteCreatedUser to update them to treat it as a non-promise-returning function
if necessary.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/maestro-android.yml.github/workflows/maestro-ios.yml.maestro/helpers/create-account.yaml.maestro/helpers/search-room.yaml.maestro/scripts/data-setup.js.maestro/tests/assorted/broadcast.yaml.maestro/tests/assorted/change-avatar.yaml.maestro/tests/e2ee/flows/create-e2ee-room.yaml.maestro/tests/e2ee/utils/change-e2ee-key.yaml.maestro/tests/onboarding/change-password.yaml.maestro/tests/onboarding/login/invalid-credentials.yaml.maestro/tests/onboarding/login/login.yaml.maestro/tests/onboarding/register/create-account.yaml.maestro/tests/room/create-room.yaml.maestro/tests/room/ignoreuser.yaml.maestro/tests/room/room.yaml.maestro/tests/room/share-message.yaml.maestro/tests/teams/team.yaml.maestro/tests/teams/utils/create-channel.yamlapp/views/ReportUserView/index.tsxscripts/create-avd.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: E2E Build Android / android-build
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7008
File: .github/workflows/maestro-ios.yml:90-93
Timestamp: 2026-02-26T10:48:56.700Z
Learning: In the Rocket.Chat.ReactNative repository's Maestro iOS workflow (.github/workflows/maestro-ios.yml), the `retry_on: timeout` configuration is intentionally used instead of `retry_on: any` because test failures are handled within the .github/scripts/run-maestro.sh script itself. Workflow-level retries should only occur when Maestro hangs (timeout), not on test failures.
📚 Learning: 2025-12-17T15:56:22.578Z
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
Applied to files:
.maestro/tests/room/ignoreuser.yaml
📚 Learning: 2026-02-26T10:48:56.700Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7008
File: .github/workflows/maestro-ios.yml:90-93
Timestamp: 2026-02-26T10:48:56.700Z
Learning: In the Rocket.Chat.ReactNative repository's Maestro iOS workflow (.github/workflows/maestro-ios.yml), the `retry_on: timeout` configuration is intentionally used instead of `retry_on: any` because test failures are handled within the .github/scripts/run-maestro.sh script itself. Workflow-level retries should only occur when Maestro hangs (timeout), not on test failures.
Applied to files:
.maestro/tests/room/ignoreuser.yaml.github/workflows/maestro-ios.yml
🪛 actionlint (1.7.11)
.github/workflows/maestro-ios.yml
[error] 83-83: shellcheck reported issue in this script: SC2086:info:3:22: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (17)
.maestro/helpers/search-room.yaml (1)
8-9: LGTM!Adding
waitForAnimationToEndbefore interacting with the search element is a good practice for reducing test flakiness. This ensures the rooms list view has finished any entrance/layout animations before attempting to tap on the search button, which aligns well with the PR's goal of improving test reliability.app/views/ReportUserView/index.tsx (1)
90-90: LGTM!Adding
keyboardShouldPersistTaps='always'is a good change that aligns with the PR's goal of improving E2E test reliability. This ensures taps on the submit button are registered even when the keyboard is visible, eliminating the need for explicit keyboard dismissal in Maestro tests and improving the user experience for form submission.scripts/create-avd.sh (4)
1-15: LGTM!The environment setup with fallback logic for
ANDROID_HOMEand PATH augmentation is well-structured. Theset -eensures early failure detection, and debug output aids CI troubleshooting.
17-31: LGTM!The dynamic architecture detection correctly handles both macOS (
arm64) and Linux (aarch64) ARM identifiers, ensuring compatibility across different CI runners. The IMAGE string construction is clean and maintainable.
46-51: Appending config may create duplicates on re-runs.Using
>>to append toconfig.iniwill add duplicate entries if the script runs multiple times (e.g., when AVD cache exists from a previous run).
53-53: LGTM!Clear confirmation output with relevant diagnostic information.
.maestro/tests/teams/utils/create-channel.yaml (1)
37-37: LGTM!The change correctly delegates keyboard hiding to the shared helper flow, improving reliability for iOS keyboard dismissal as intended by the PR objectives.
.maestro/helpers/create-account.yaml (1)
27-27: LGTM!Correctly references the hide-keyboard helper in the same directory.
.maestro/tests/onboarding/register/create-account.yaml (1)
31-31: LGTM!Consistent with the broader refactor to use the shared keyboard-hiding helper.
.maestro/tests/e2ee/utils/change-e2ee-key.yaml (1)
10-10: LGTM!Correctly delegates to the shared keyboard-hiding helper.
.maestro/tests/room/share-message.yaml (1)
71-71: LGTM!Correctly delegates to the shared keyboard-hiding helper with the appropriate relative path.
.maestro/tests/onboarding/login/invalid-credentials.yaml (1)
20-20: LGTM!Consistent keyboard-hiding refactor with correct path.
.maestro/scripts/data-setup.js (1)
217-252: Well-structured retry mechanism with exponential backoff.The implementation correctly:
- Returns immediately on 2xx success
- Fails fast on 4xx client errors (non-retryable)
- Retries on 5xx server errors and network failures
- Uses exponential backoff to avoid overwhelming the server
This addresses the PR objective of making workspace API calls more robust.
.maestro/tests/onboarding/change-password.yaml (1)
29-35: LGTM!Both keyboard-hiding steps correctly delegate to the shared helper flow, consistent with the broader refactor pattern.
.maestro/tests/room/create-room.yaml (2)
174-174: LGTM — consistent use of the shared keyboard-hiding helper.The replacement of inline
hideKeyboardwithrunFlow: '../../helpers/hide-keyboard.yaml'across all four locations is consistent and aligns with the PR objective to improve iOS keyboard dismissal reliability through a reusable helper flow.Also applies to: 193-193, 261-261, 325-325
1-9: The AI-generated summary appears to be incorrect. There is no evidence that a test titled "should create a room with non-latin alphabet and do a case insensitive search for it" was removed from this file. A search of the entire maestro directory found no references to "non-latin", "alphabet", or "case insensitive search" tests. The current create-room.yaml file contains test scenarios for public rooms, private rooms, empty rooms, user search, and selection—but no non-Latin alphabet test exists or existed in the visible history of this file.Likely an incorrect or invalid review comment.
.maestro/tests/onboarding/login/login.yaml (1)
21-21: LGTM — helper flow path is correct.The replacement of inline
hideKeyboardwith the helper flow is consistent with the broader pattern in this PR. The relative path../../../helpers/hide-keyboard.yamlcorrectly resolves to.maestro/helpers/hide-keyboard.yamlfrom this file's location.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/maestro-ios.yml (1)
84-87:⚠️ Potential issue | 🟡 MinorQuote
$GITHUB_ENVin redirection target.Please change
>> $GITHUB_ENVto>> "$GITHUB_ENV"for safe shell redirection handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/maestro-ios.yml around lines 84 - 87, Change the shell redirection to safely handle paths with spaces by quoting the environment file variable: update the redirection that appends UDID into $GITHUB_ENV so it uses "$GITHUB_ENV" (the line referencing UDID and appending to GITHUB_ENV), ensuring the echo "UDID=$UDID" >> "$GITHUB_ENV" form is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.maestro/scripts/data-setup.js:
- Around line 231-238: The 4xx branch currently throws an Error (`Non-retryable
error ${response.status}`) but the outer catch immediately captures it (setting
lastError) and the retry loop continues; change the control flow so
non-retryable 4xx errors cause an immediate exit by rethrowing the error (or
returning/breaking) instead of letting the catch swallow it—modify the response
status check block and ensure the catch only handles retryable errors while
letting the thrown non-retryable Error propagate (or rethrow within catch) so
the loop stops; key symbols to update are the response status check that throws
`Non-retryable error ${response.status}`, the `catch (err)` block that sets
`lastError`, and the `lastError` handling logic.
- Around line 250-252: The POST helper postWithRetry currently wraps all
http.post calls with retryRequest which risks duplicating non-idempotent
operations; change the default so POSTs do not retry: stop using retryRequest
inside postWithRetry (or replace postWithRetry with a plain post helper that
calls http.post directly) and only apply retryRequest for specific
safe/idempotent operations (create a separate postWithRetrySafe or allow an
explicit option like {enableRetry: true, idempotencyKey}) while leaving
getWithRetry unchanged; update call sites (e.g., users.create, teams.create,
chat.postMessage) to use the non-retrying POST helper unless the call is
explicitly safe or supplies an idempotency key.
---
Duplicate comments:
In @.github/workflows/maestro-ios.yml:
- Around line 84-87: Change the shell redirection to safely handle paths with
spaces by quoting the environment file variable: update the redirection that
appends UDID into $GITHUB_ENV so it uses "$GITHUB_ENV" (the line referencing
UDID and appending to GITHUB_ENV), ensuring the echo "UDID=$UDID" >>
"$GITHUB_ENV" form is used.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/maestro-ios.yml.maestro/scripts/data-setup.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7008
File: .github/workflows/maestro-ios.yml:90-93
Timestamp: 2026-02-26T10:48:56.700Z
Learning: In the Rocket.Chat.ReactNative repository's Maestro iOS workflow (.github/workflows/maestro-ios.yml), the `retry_on: timeout` configuration is intentionally used instead of `retry_on: any` because test failures are handled within the .github/scripts/run-maestro.sh script itself. Workflow-level retries should only occur when Maestro hangs (timeout), not on test failures.
📚 Learning: 2026-02-26T10:48:56.700Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7008
File: .github/workflows/maestro-ios.yml:90-93
Timestamp: 2026-02-26T10:48:56.700Z
Learning: In the Rocket.Chat.ReactNative repository's Maestro iOS workflow (.github/workflows/maestro-ios.yml), the `retry_on: timeout` configuration is intentionally used instead of `retry_on: any` because test failures are handled within the .github/scripts/run-maestro.sh script itself. Workflow-level retries should only occur when Maestro hangs (timeout), not on test failures.
Applied to files:
.github/workflows/maestro-ios.yml
🧬 Code graph analysis (1)
.maestro/scripts/data-setup.js (2)
.maestro/scripts/data.js (1)
data(1-16)app/lib/methods/helpers/fetch.ts (1)
url(42-54)
🔇 Additional comments (1)
.github/workflows/maestro-ios.yml (1)
13-14: No action needed—theMAESTRO_VERSIONenvironment variable is properly honored by the installer.The maestro installer script checks for
MAESTRO_VERSIONand uses a specific release URL when set (e.g.,cli-2.2.0), or falls back to "latest" if unset. Since the job env definesMAESTRO_VERSION: 2.2.0, the installer will correctly pin to that version. The workflow is configured correctly.
2 participants
Proposed changes
Right now, our end-to-end tests are flaky and require multiple runs for all tests to pass. This PR improves the behavior by introducing the following changes:
Result:
95% of the time, all Android tests pass on the first run, and iOS tests pass on the first run about 90% of the time.
Note: The Avatar test is currently failing due to a backend issue.
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-1864
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
After merging this PR, we will monitor which tests are still flaky and fix them accordingly.
Summary by CodeRabbit
Performance Improvements
Quality Improvements
Bug Fixes