Issue #194: Add cross-cutting E2E policy, guardrails, and CI lanes#222
Merged
Conversation
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds comprehensive E2E infrastructure: Playwright web tests and config, iOS XCUITest runner and workflows, CI policy gates and artifact uploads, repository guard and perf-reporting scripts, run-scoped seeding, docs/runbooks, .env example placeholders, and npm scripts to orchestrate lanes. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions (CI)
participant Guard as Policy Scripts
participant Repo as Repository
participant Server as Dev Server
participant Playwright as Playwright Runner
participant App as Web App
participant Art as Artifact Store
CI->>Repo: checkout code + set envs
CI->>Guard: run e2e:policy checks (assert-non-prod, no-naked-sleep, check-test-secrets)
Guard-->>CI: pass/fail
CI->>Server: start or reuse dev server (npm run dev)
Server-->>CI: server ready
CI->>Playwright: run npx playwright test --grep <lane>
Playwright->>App: HTTP requests to E2E_BASE_URL
App-->>Playwright: responses
Playwright-->>CI: exit code + artifacts (trace/video/screenshots)
CI->>Art: upload artifacts & perf reports
sequenceDiagram
participant CI as GitHub Actions (CI)
participant Runner as run-ios-tests.sh
participant Seed as seed.ts
participant Xcode as xcodebuild
participant Perf as report-ios-perf
participant Art as Artifact Store
CI->>Runner: invoke lane (smoke/critical) with envs
Runner->>Seed: seed fixtures using E2E_RUN_ID
Seed-->>Runner: seeded test data
Runner->>Runner: assert-non-prod & secrets checks
Runner->>Xcode: run xcodebuild test -only-testing <target>
Xcode-->>Runner: xcresult/logs + exit code
alt retry on failure
Runner->>Xcode: retry attempt
end
Runner->>Perf: compute/write perf-summary.json
Runner->>Art: upload xcresult/logs/perf artifacts
Runner-->>CI: final exit status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
scripts/e2e/run-playwright-lane.mjs (1)
5-11: Minor: Function nameparsePositiveIntaccepts zero.The check
parsed < 0allows zero through, but the name suggests only positive integers. This is fine for the use case (0 retries/workers is valid), but consider renaming toparseNonNegativeIntfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/e2e/run-playwright-lane.mjs` around lines 5 - 11, The function parsePositiveInt allows zero but its name implies strictly positive values; rename the function to parseNonNegativeInt (and update all call sites) to reflect that it accepts zero, and update any documentation/comments accordingly; ensure the signature and behavior (using Number.parseInt, checking Number.isFinite(parsed) || parsed < 0) remain unchanged except for the identifier change so existing logic for retries/workers continues to work.ios/E2E_RUNBOOK.md (1)
15-15: Consider using a more flexible simulator destination.
name=iPhone 16is specific to a particular device model. Consider using a more flexible destination likename=iPhone 16 Proor documenting that this should be adjusted based on available simulators. Alternatively, use a generic pattern in docs and let CI specify the exact device.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/E2E_RUNBOOK.md` at line 15, Update the IOS_TEST_DESTINATION example to use a more flexible simulator destination or document that it should be adjusted per environment: replace the explicit `name=iPhone 16` example in the IOS_TEST_DESTINATION entry with a more general pattern (e.g., a broader device name such as an iPhone 16 Pro or a pattern like name=iPhone*) or add a note that CI/maintainers should pick an available simulator and adjust IOS_TEST_DESTINATION accordingly; ensure the documentation line references the variable IOS_TEST_DESTINATION so readers know where to make the change.scripts/e2e/assert-non-prod.mjs (1)
10-18: Minor:parseHostsilently returnsnullon invalid URLs.This is handled correctly downstream (filtered out at line 29), but consider logging a warning when a URL fails to parse, as it could indicate a configuration typo that's silently ignored.
♻️ Proposed enhancement
function parseHost(rawUrl) { if (!rawUrl) { return null; } try { return new URL(rawUrl).hostname.toLowerCase(); - } catch { + } catch (err) { + console.warn(`Warning: Could not parse URL "${rawUrl}": ${err.message}`); return null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/e2e/assert-non-prod.mjs` around lines 10 - 18, The parseHost function currently swallows invalid URLs and returns null; modify parseHost to log a warning (including the rawUrl value) when URL parsing throws so configuration typos are visible; keep the existing return null behavior so downstream filtering (where nulls are ignored) still works—update the catch block inside parseHost to emit a concise warning via the project's logger or console.warn mentioning rawUrl and that parsing failed..github/workflows/e2e-ios.yml (1)
14-133: Consider reducing duplication between smoke and critical jobs.The two jobs are nearly identical, differing only in lane name and artifact name. Consider using a matrix strategy to DRY this up:
♻️ Matrix-based refactor
jobs: ios-e2e: name: ios-e2e-${{ matrix.lane }} runs-on: macos-26 timeout-minutes: 45 strategy: fail-fast: false matrix: lane: [smoke, critical] steps: # ... shared steps ... - name: Run iOS ${{ matrix.lane }} lane env: # ... env vars ... run: npm run e2e:ios:${{ matrix.lane }} - name: Upload iOS E2E artifacts if: always() uses: actions/upload-artifact@v4 with: name: e2e-ios-${{ matrix.lane }}-artifacts path: artifacts/e2e/ios/** # ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-ios.yml around lines 14 - 133, Replace the duplicated ios-e2e-smoke and ios-e2e-critical jobs with a single matrix job (e.g., ios-e2e) that uses strategy.matrix.lane: [smoke, critical] and fail-fast: false; consolidate the shared steps (Checkout, Setup Node.js, Install dependencies, Install XcodeGen, Generate Xcode project, Record perf, Upload artifacts) and parameterize the lane-specific parts using matrix.lane for the Run step (npm run e2e:ios:${{ matrix.lane }}) and for the artifact name (e2e-ios-${{ matrix.lane }}-artifacts) so the two lanes run from the same job definition while preserving identical env variables and artifact path.scripts/e2e/check-no-naked-sleep.mjs (1)
6-8: Allowlist is narrowly scoped — document or consider generalizing.The allowlist only permits
sleep $((attempt * 10))exactly. If a different bounded backoff pattern is used (e.g.,sleep $((attempt * 5))), it will be flagged as a violation. This is fine if intentional, but consider documenting this constraint in the policy doc or generalizing the pattern to/sleep\s+\$\(\(attempt \* \d+\)\)/if other bounded formulas should be allowed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/e2e/check-no-naked-sleep.mjs` around lines 6 - 8, The ALLOWED_SLEEP_PATTERNS entry currently matches only the literal `sleep $((attempt * 10))`, which will miss other bounded backoff variants; update the allowlist in scripts/e2e/check-no-naked-sleep.mjs by either (a) generalizing the regex in ALLOWED_SLEEP_PATTERNS to something like `/sleep\s+\$\(\(attempt \* \d+\)\)/` to allow any numeric multiplier, or (b) keep the specific pattern but add a clear comment/docstring next to the ALLOWED_SLEEP_PATTERNS constant explaining the intentional narrow scope so reviewers know this is by design; ensure you modify the ALLOWED_SLEEP_PATTERNS constant and its inline comment accordingly..github/workflows/e2e-web.yml (2)
111-116: Placeholder performance metric value.
E2E_WEB_FIRST_RESPONSE_MS: "800"is a hardcoded placeholder, not an actual measured value from the tests. While this is report-only mode (E2E_WEB_PERF_ENFORCE: "false"), the resultingweb-metric.jsonartifact may be misleading.Consider either:
- Adding a comment clarifying this is a baseline placeholder
- Skipping the perf step until actual measurement instrumentation is in place
- Documenting in the policy that v1 metrics are baseline placeholders
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-web.yml around lines 111 - 116, The workflow step "Record web performance metric (report-only by default)" uses a hardcoded placeholder value E2E_WEB_FIRST_RESPONSE_MS: "800" which can be misleading; update the e2e-web.yml to either (a) add an inline comment directly above that job/step clarifying that E2E_WEB_FIRST_RESPONSE_MS and E2E_WEB_FIRST_RESPONSE_THRESHOLD_MS are baseline placeholders used only for reporting when E2E_WEB_PERF_ENFORCE is "false", or (b) gate the step behind a boolean input/secret so it is skipped until you have real instrumentation (e.g., only run when a workflow input like run_perf == true), and ensure the comment references the env names E2E_WEB_FIRST_RESPONSE_MS, E2E_WEB_FIRST_RESPONSE_THRESHOLD_MS and E2E_WEB_PERF_ENFORCE so future readers know the intent.
60-64: Redundant guard execution.The
npm run e2e:guardstep on line 61 is redundant becausenpm run e2e:web:smoke(line 64) already runse2e:guardas its first command (seepackage.jsonline 16:npm run e2e:guard && node scripts/e2e/run-playwright-lane.mjs ...).Consider removing the explicit step to reduce workflow runtime:
- - name: Assert non-production target - run: npm run e2e:guard - - name: Run web smoke lane run: npm run e2e:web:smokeThe same applies to lines 105-109 in the critical job.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-web.yml around lines 60 - 64, Remove the redundant explicit "Assert non-production target" step that runs `npm run e2e:guard` because the following "Run web smoke lane" step already invokes `npm run e2e:guard` as part of `npm run e2e:web:smoke`; delete the `- name: Assert non-production target` step (the `npm run e2e:guard` run) before `Run web smoke lane` and do the same removal for the equivalent steps in the critical job (the block around lines where the critical job runs `npm run e2e:guard` followed by `npm run e2e:web:smoke`) so the workflow only runs the guard once per lane.scripts/e2e/run-ios-tests.sh (2)
23-26: Bash syntax issue: compound condition needs different grouping.The condition on line 23 uses
(...)which spawns a subshell rather than grouping. For conditional grouping in[[ ]], use&&directly or{ ...; }for command grouping. However, this specific construct should work because it's within[[ ]], but the spacing and structure could be clearer.Consider simplifying:
-if [[ "${SECRETS_REQUIRED}" == "true" && ("${E2E_TEST_USER_EMAIL:-}" == "" || "${E2E_TEST_USER_PASSWORD:-}" == "") ]]; then +if [[ "${SECRETS_REQUIRED}" == "true" ]] && [[ -z "${E2E_TEST_USER_EMAIL:-}" || -z "${E2E_TEST_USER_PASSWORD:-}" ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/e2e/run-ios-tests.sh` around lines 23 - 26, The compound bash condition is fragile; replace the current if test with a clearer check using -z and proper grouping inside [[ ]]. Change the if to: if [[ "${SECRETS_REQUIRED}" == "true" && ( -z "${E2E_TEST_USER_EMAIL:-}" || -z "${E2E_TEST_USER_PASSWORD:-}" ) ]]; then so you test emptiness with -z for E2E_TEST_USER_EMAIL and E2E_TEST_USER_PASSWORD and group the OR check correctly (references: SECRETS_REQUIRED, E2E_TEST_USER_EMAIL, E2E_TEST_USER_PASSWORD and the if condition).
36-36: Misleading variable name:WORKSPACE_PATHholds a project path.The variable is named
WORKSPACE_PATHbut defaults to a.xcodeproj(project) and is used with-projectflag on line 61. In Xcode terminology,.xcworkspaceis a workspace and.xcodeprojis a project.Consider renaming for clarity:
-WORKSPACE_PATH="${IOS_TEST_WORKSPACE:-ios/StillPoint.xcodeproj}" +PROJECT_PATH="${IOS_TEST_PROJECT:-ios/StillPoint.xcodeproj}"And update the env var name from
IOS_TEST_WORKSPACEtoIOS_TEST_PROJECT, plus the usage on line 61.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/e2e/run-ios-tests.sh` at line 36, The variable WORKSPACE_PATH is misnamed because it holds an Xcode project (.xcodeproj) and is used with the -project flag; rename WORKSPACE_PATH to PROJECT_PATH (or PROJECT_FILE) and change the environment variable from IOS_TEST_WORKSPACE to IOS_TEST_PROJECT, then update every reference (including the -project invocation that currently uses WORKSPACE_PATH and any exports/usages) to use PROJECT_PATH/IOS_TEST_PROJECT instead to make intent clear.
🤖 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/assert-non-prod.mjs`:
- Line 8: The current blockedPatterns array includes /\.vercel\.app$/i which
prevents running E2E on Vercel previews; update the logic that defines
blockedPatterns in scripts/e2e/assert-non-prod.mjs so the Vercel pattern is not
applied when previews should be allowed (e.g., honor an env var like
E2E_ALLOW_VERCEL_PREVIEW=true) or remove the /\.vercel\.app$/i entry entirely if
previews are valid test targets; locate the blockedPatterns declaration and
conditionally push the /\.vercel\.app$/i pattern only when
process.env.E2E_ALLOW_VERCEL_PREVIEW is not set to 'true' (or replace with a
more specific production-only domain if available).
In `@scripts/e2e/report-ios-perf.mjs`:
- Around line 32-35: The current logic assigns 0 when IOS_E2E_APP_BOOT_SECONDS
and IOS_E2E_APP_BOOT_TIME are absent/invalid; change it to fail fast instead: in
the uiBootSeconds computation (the const using parseDurationToSeconds and env
vars IOS_E2E_APP_BOOT_SECONDS / IOS_E2E_APP_BOOT_TIME) detect when both parses
return null/undefined and throw an explicit Error or exit with a non‑zero status
and a clear message indicating the missing boot metric; apply the same failing
behavior to the other similar metric computed with parseDurationToSeconds on the
alternate env var at the other occurrence (the one mentioned for line 45) so
missing metrics do not default to 0.
- Around line 25-27: The current parsing uses Number.parseFloat(value) which
accepts partial prefixes like "12abc"; update the parsing to first validate that
the input string (value) is a well-formed numeric literal (e.g. match a regex
for integers/floats with optional sign and decimal, no trailing characters)
before converting; if the validation fails return null, otherwise parse to a
Number (keep using Number.parseFloat or Number(value)) and keep the existing
Number.isFinite(numeric) check to return numeric or null. Ensure you update the
function around the numeric/Number.parseFloat(value) usage and reference the
variables/function where numeric and value are used.
In `@scripts/e2e/report-web-perf.mjs`:
- Around line 17-18: Validate the parsed threshold before using it: ensure
thresholdMs (from Number(process.env.E2E_WEB_FIRST_RESPONSE_THRESHOLD_MS ??
"1200")) is a finite positive number and, if not, refuse to run enforcement when
thresholdEnforced is true. Update the code that sets thresholdMs and
thresholdEnforced to parse and check Number.isFinite(thresholdMs) && thresholdMs
> 0, and when thresholdEnforced is true but the threshold is invalid, log a
clear error and exit (or set thresholdEnforced to false) so the regression guard
(the block that uses thresholdEnforced) cannot be silently bypassed.
In `@scripts/e2e/run-playwright-lane.mjs`:
- Around line 33-34: The script run-playwright-lane.mjs is forcing Playwright's
reporter via the hardcoded flags "--reporter","line" which overrides the
CI-aware reporters in playwright.config.ts; remove those two entries (or wrap
them behind a non-CI condition using process.env.CI) so the config's reporters
take precedence, or if you want explicit control replace them with the CI
reporters you need (e.g., "github" and "html") when CI is true; look for the
array or function building the Playwright CLI args in run-playwright-lane.mjs
and update the handling of "--reporter" accordingly.
---
Nitpick comments:
In @.github/workflows/e2e-ios.yml:
- Around line 14-133: Replace the duplicated ios-e2e-smoke and ios-e2e-critical
jobs with a single matrix job (e.g., ios-e2e) that uses strategy.matrix.lane:
[smoke, critical] and fail-fast: false; consolidate the shared steps (Checkout,
Setup Node.js, Install dependencies, Install XcodeGen, Generate Xcode project,
Record perf, Upload artifacts) and parameterize the lane-specific parts using
matrix.lane for the Run step (npm run e2e:ios:${{ matrix.lane }}) and for the
artifact name (e2e-ios-${{ matrix.lane }}-artifacts) so the two lanes run from
the same job definition while preserving identical env variables and artifact
path.
In @.github/workflows/e2e-web.yml:
- Around line 111-116: The workflow step "Record web performance metric
(report-only by default)" uses a hardcoded placeholder value
E2E_WEB_FIRST_RESPONSE_MS: "800" which can be misleading; update the e2e-web.yml
to either (a) add an inline comment directly above that job/step clarifying that
E2E_WEB_FIRST_RESPONSE_MS and E2E_WEB_FIRST_RESPONSE_THRESHOLD_MS are baseline
placeholders used only for reporting when E2E_WEB_PERF_ENFORCE is "false", or
(b) gate the step behind a boolean input/secret so it is skipped until you have
real instrumentation (e.g., only run when a workflow input like run_perf ==
true), and ensure the comment references the env names
E2E_WEB_FIRST_RESPONSE_MS, E2E_WEB_FIRST_RESPONSE_THRESHOLD_MS and
E2E_WEB_PERF_ENFORCE so future readers know the intent.
- Around line 60-64: Remove the redundant explicit "Assert non-production
target" step that runs `npm run e2e:guard` because the following "Run web smoke
lane" step already invokes `npm run e2e:guard` as part of `npm run
e2e:web:smoke`; delete the `- name: Assert non-production target` step (the `npm
run e2e:guard` run) before `Run web smoke lane` and do the same removal for the
equivalent steps in the critical job (the block around lines where the critical
job runs `npm run e2e:guard` followed by `npm run e2e:web:smoke`) so the
workflow only runs the guard once per lane.
In `@ios/E2E_RUNBOOK.md`:
- Line 15: Update the IOS_TEST_DESTINATION example to use a more flexible
simulator destination or document that it should be adjusted per environment:
replace the explicit `name=iPhone 16` example in the IOS_TEST_DESTINATION entry
with a more general pattern (e.g., a broader device name such as an iPhone 16
Pro or a pattern like name=iPhone*) or add a note that CI/maintainers should
pick an available simulator and adjust IOS_TEST_DESTINATION accordingly; ensure
the documentation line references the variable IOS_TEST_DESTINATION so readers
know where to make the change.
In `@scripts/e2e/assert-non-prod.mjs`:
- Around line 10-18: The parseHost function currently swallows invalid URLs and
returns null; modify parseHost to log a warning (including the rawUrl value)
when URL parsing throws so configuration typos are visible; keep the existing
return null behavior so downstream filtering (where nulls are ignored) still
works—update the catch block inside parseHost to emit a concise warning via the
project's logger or console.warn mentioning rawUrl and that parsing failed.
In `@scripts/e2e/check-no-naked-sleep.mjs`:
- Around line 6-8: The ALLOWED_SLEEP_PATTERNS entry currently matches only the
literal `sleep $((attempt * 10))`, which will miss other bounded backoff
variants; update the allowlist in scripts/e2e/check-no-naked-sleep.mjs by either
(a) generalizing the regex in ALLOWED_SLEEP_PATTERNS to something like
`/sleep\s+\$\(\(attempt \* \d+\)\)/` to allow any numeric multiplier, or (b)
keep the specific pattern but add a clear comment/docstring next to the
ALLOWED_SLEEP_PATTERNS constant explaining the intentional narrow scope so
reviewers know this is by design; ensure you modify the ALLOWED_SLEEP_PATTERNS
constant and its inline comment accordingly.
In `@scripts/e2e/run-ios-tests.sh`:
- Around line 23-26: The compound bash condition is fragile; replace the current
if test with a clearer check using -z and proper grouping inside [[ ]]. Change
the if to: if [[ "${SECRETS_REQUIRED}" == "true" && ( -z
"${E2E_TEST_USER_EMAIL:-}" || -z "${E2E_TEST_USER_PASSWORD:-}" ) ]]; then so you
test emptiness with -z for E2E_TEST_USER_EMAIL and E2E_TEST_USER_PASSWORD and
group the OR check correctly (references: SECRETS_REQUIRED, E2E_TEST_USER_EMAIL,
E2E_TEST_USER_PASSWORD and the if condition).
- Line 36: The variable WORKSPACE_PATH is misnamed because it holds an Xcode
project (.xcodeproj) and is used with the -project flag; rename WORKSPACE_PATH
to PROJECT_PATH (or PROJECT_FILE) and change the environment variable from
IOS_TEST_WORKSPACE to IOS_TEST_PROJECT, then update every reference (including
the -project invocation that currently uses WORKSPACE_PATH and any
exports/usages) to use PROJECT_PATH/IOS_TEST_PROJECT instead to make intent
clear.
In `@scripts/e2e/run-playwright-lane.mjs`:
- Around line 5-11: The function parsePositiveInt allows zero but its name
implies strictly positive values; rename the function to parseNonNegativeInt
(and update all call sites) to reflect that it accepts zero, and update any
documentation/comments accordingly; ensure the signature and behavior (using
Number.parseInt, checking Number.isFinite(parsed) || parsed < 0) remain
unchanged except for the identifier change so existing logic for retries/workers
continues to work.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e08822c8-d093-4b74-b402-faaa9c3b7549
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.env.example.github/workflows/e2e-ios.yml.github/workflows/e2e-web.yml.gitignoreREADME.mddocs/testing/e2e-policy.mde2e/web/critical.spec.tse2e/web/smoke.spec.tsios/E2E_RUNBOOK.mdios/RELEASING.mdpackage.jsonplaywright.config.tsscripts/e2e/assert-non-prod.mjsscripts/e2e/check-no-naked-sleep.mjsscripts/e2e/check-test-secrets.mjsscripts/e2e/report-ios-perf.mjsscripts/e2e/report-web-perf.mjsscripts/e2e/run-ios-tests.shscripts/e2e/run-playwright-lane.mjsscripts/seed.ts
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/check-no-naked-sleep.mjs`:
- Around line 49-53: Currently the try/catch around await walk(full, files)
swallows all errors; change the catch to only ignore missing-directory errors
and rethrow all others by checking the thrown error's code (e.g., err &&
err.code === 'ENOENT') or equivalent; update the catch block for the walk(full,
files) call so it returns/continues only when the error indicates a non-existent
optional directory and throws the error for permission/I/O or other failures.
- Around line 18-26: The hasNakedSleep function's detection is too narrow:
update the shell sleep regex to catch calls like sleep $((...)), sleep $VAR,
sleep "..." as well as sleep <number> by replacing /\bsleep\(|\bsleep\s+\d+/
with a single pattern that matches sleep followed by paren or any
whitespace+argument (e.g. /\bsleep(?:\s*\(|\s+[^\s]+)/). Also broaden the Swift
test path detection used to build swiftSleepPattern: change the isSwiftTestFile
regex from /\/Tests\/.*Tests\.swift$/i to a pattern that matches both Tests and
UITests directories (for example, detect path segments ending in Tests or
UITests and a .swift file) so swiftSleepPattern still checks
/\bThread\.sleep\b|\bTask\.sleep\b/ for those files. Ensure you update the
identifiers hasNakedSleep, unixPath, isSwiftTestFile, and swiftSleepPattern
accordingly.
In `@scripts/e2e/run-ios-tests.sh`:
- Line 5: MAX_RETRIES can be zero or non-numeric which allows skipping tests or
breaking comparisons; add an upfront validation for the MAX_RETRIES variable
before the retry loop: ensure MAX_RETRIES is a positive integer (>=1) by
checking it matches a digits-only pattern and converting/validating its numeric
value, and if validation fails or value < 1 print an error and exit non-zero;
reference the MAX_RETRIES variable and the retry loop in the script so the guard
runs prior to entering the loop.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37614ec5-dd55-4dcf-9606-c975d9ec5d46
📒 Files selected for processing (10)
.github/workflows/e2e-ios.yml.github/workflows/e2e-web.ymldocs/testing/e2e-policy.mdios/E2E_RUNBOOK.mdscripts/e2e/assert-non-prod.mjsscripts/e2e/check-no-naked-sleep.mjsscripts/e2e/report-ios-perf.mjsscripts/e2e/report-web-perf.mjsscripts/e2e/run-ios-tests.shscripts/e2e/run-playwright-lane.mjs
✅ Files skipped from review due to trivial changes (3)
- scripts/e2e/report-web-perf.mjs
- .github/workflows/e2e-ios.yml
- .github/workflows/e2e-web.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/e2e/assert-non-prod.mjs
- scripts/e2e/report-ios-perf.mjs
- scripts/e2e/run-playwright-lane.mjs
- ios/E2E_RUNBOOK.md
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/e2e/run-ios-tests.sh (1)
20-26: Consider writing status file on pass/fail for consistency.The
${LANE}.statusfile is written on skip (line 24), but not when tests pass (line 82) or fail (line 87). If downstream tooling expects a status file to always be present, this inconsistency could cause issues. Exit codes are sufficient for CI gating, so this is optional.♻️ Proposed fix for status file consistency
if run_lane "${LANE}"; then echo "iOS ${LANE} lane passed." + echo "passed" > "artifacts/e2e/ios/${LANE}.status" exit 0 fi if [[ "$ATTEMPT" -ge "$MAX_RETRIES" ]]; then echo "iOS ${LANE} lane failed after ${MAX_RETRIES} attempt(s)." + echo "failed" > "artifacts/e2e/ios/${LANE}.status" exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/e2e/run-ios-tests.sh` around lines 20 - 26, The script currently writes "artifacts/e2e/ios/${LANE}.status" only on skip; update run-ios-tests.sh to also write a status file on test success and failure so downstream tooling always finds a status. Specifically, when tests finish successfully write "passed" to "artifacts/e2e/ios/${LANE}.status" (near the current success/exit path, e.g. where the script currently exits at the success branch around the current end of tests), and when tests fail write "failed" to that same path (near the failure branch where the script currently exits with non-zero around the existing failure handling). Use the same ${LANE} and artifacts directory and keep the existing mkdir -p behavior; alternatively add a small exit trap that writes "failed" on non-zero exit and write "passed" on normal completion to ensure coverage for unexpected exits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/e2e/run-ios-tests.sh`:
- Around line 20-26: The script currently writes
"artifacts/e2e/ios/${LANE}.status" only on skip; update run-ios-tests.sh to also
write a status file on test success and failure so downstream tooling always
finds a status. Specifically, when tests finish successfully write "passed" to
"artifacts/e2e/ios/${LANE}.status" (near the current success/exit path, e.g.
where the script currently exits at the success branch around the current end of
tests), and when tests fail write "failed" to that same path (near the failure
branch where the script currently exits with non-zero around the existing
failure handling). Use the same ${LANE} and artifacts directory and keep the
existing mkdir -p behavior; alternatively add a small exit trap that writes
"failed" on non-zero exit and write "passed" on normal completion to ensure
coverage for unexpected exits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12421f41-7484-4e7c-bf93-488accfb736c
📒 Files selected for processing (2)
scripts/e2e/check-no-naked-sleep.mjsscripts/e2e/run-ios-tests.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/e2e/check-no-naked-sleep.mjs
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
This was referenced Apr 24, 2026
This was referenced Apr 28, 2026
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
2 participants
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
Implements the cross-cutting E2E testing policy baseline for web and iOS in this repository, including executable guardrails and CI lane wiring.
What changed
docs/testing/e2e-policy.mdios/E2E_RUNBOOK.mdplaywright.config.tse2e/web/smoke.spec.tse2e/web/critical.spec.tsscripts/e2e/assert-non-prod.mjsscripts/e2e/check-no-naked-sleep.mjsscripts/e2e/check-test-secrets.mjsscripts/e2e/run-playwright-lane.mjsscripts/e2e/run-ios-tests.shscripts/e2e/report-web-perf.mjsscripts/e2e/report-ios-perf.mjs.github/workflows/e2e-web.yml.github/workflows/e2e-ios.ymlpackage.json/package-lock.json.gitignorescripts/seed.tsnow supportsE2E_RUN_IDfor run-unique users.env.examplewith E2E placeholders only (no real creds)Follow-up hardening after initial CI + review feedback
E2E_SECRETS_REQUIREDfallback behavior.smoke,critical) to reduce duplication.e2e:guard.E2E_ALLOW_VERCEL_PREVIEW=trueand warns on unparsable URL inputs.parsePositiveInt->parseNonNegativeInt.--reporter lineso config reporters are respected.PROJECT_PATH/IOS_TEST_PROJECT) and improved secret-check condition readability.passed/failed/skipped) for downstream tooling.Merge maintenance
origin/maininto this branch.ios/RELEASING.mdby preserving both:Acceptance criteria coverage
E2E_RUN_IDsupport inscripts/seed.tsand documented disposable/reset strategy.e2e:policy:no-sleepscript..env.example; CI/local env-only requirement and checker script.e2e:guardand iOS lane guardrails.@smoke,@critical,@visual,@perfpolicy + lane wiring.Validation run
npm run e2e:policynpm run e2e:web:smokenpm run e2e:web:criticalE2E_WEB_FIRST_RESPONSE_MS=800 E2E_WEB_FIRST_RESPONSE_THRESHOLD_MS=1200 E2E_WEB_PERF_ENFORCE=false npm run e2e:web:perfIOS_E2E_APP_BOOT_SECONDS=4.2 npm run e2e:ios:perfE2E_BASE_URL=http://127.0.0.1:3000 E2E_SECRETS_REQUIRED=false npm run e2e:ios:smoke(expected skip in this repo state becauseios/StillPointUITestsis not present)Notes
coderabbit review --prompt-onlycould not be executed in this environment (coderabbit: command not found).Summary by CodeRabbit
New Features
Tests
Documentation
Chores