Add mobile web E2E coverage for viewport, touch, safe-area, and overflow#224
Add mobile web E2E coverage for viewport, touch, safe-area, and overflow#224auerbachb wants to merge 1 commit into
Conversation
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| await expect(passwordInput).toBeFocused(); | ||
| await expectVisibleInViewport(page, passwordInput, "auth password input"); | ||
| await expectNoHorizontalOverflow(page); | ||
| }); |
There was a problem hiding this comment.
Test missing mock routes will fail in CI
Medium Severity
The "focus stays reachable on auth inputs" test destructures only { page }, so the mockApiState fixture is never initialized and installMockApiRoutes is never called. Without mock API routes, the real /api/auth/me endpoint is hit. In CI (where no POSTGRES_URL is configured), the server returns a 500, causing the app to render a "server issue" error screen instead of the login form with email/password inputs the test expects. This test will reliably fail in the new e2e-mobile.yml CI workflow.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7113269. Configure here.
|
@CodeAnt-AI review |
|
CodeAnt AI is running the review. |
|
@cursor review |
|
@graphite-app re-review |
codeant-ai
Bot
added
the
size:XL
Sequence DiagramThis PR adds Playwright mobile projects and a deterministic auth fixture so tests can drive a touch-only session flow while asserting safe tap targets, viewport visibility, and overflow behavior against a mocked API state. sequenceDiagram
participant MobileTest
participant Browser
participant WebApp
participant MockApi
MobileTest->>Browser: Open app on mobile viewport
Browser->>MockApi: Send auth sign up or login request
MockApi-->>Browser: Return authenticated user and initial state
Browser->>WebApp: Render home and session controls
MobileTest->>Browser: Tap Begin and session actions
Browser->>MockApi: Send request to create session record
MockApi-->>Browser: Return stored session data
MobileTest->>MobileTest: Assert UI layout and mock state (no overflow, tap targets)
Generated by CodeAnt AI |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const PORT = process.env.PORT ? Number(process.env.PORT) : 3000; | ||
| const baseURL = process.env.PLAYWRIGHT_BASE_URL ?? `http://127.0.0.1:${PORT}`; |
There was a problem hiding this comment.
Suggestion: process.env.PORT is coerced with Number(...) without validation, so a non-numeric value (for example from CI/env misconfiguration) becomes NaN. That produces an invalid baseURL and an invalid --port argument for the web server command, causing Playwright startup to fail before tests run. Validate the parsed port and fall back to a known default when parsing fails. [possible bug]
Severity Level: Major ⚠️
- ❌ E2E Mobile Web GitHub workflow fails on bad PORT env.
- ⚠️ Local `test:e2e` runs break under PORT misconfiguration.Steps of Reproduction ✅
1. Set a non-numeric PORT environment variable before running Playwright tests, for
example: `PORT=abc npx playwright test --project mobile-chromium-narrow` (Playwright entry
configured in `package.json:12` and `playwright.config.ts:1-4`).
2. When Playwright starts, it evaluates `const PORT = process.env.PORT ?
Number(process.env.PORT) : 3000;` at `playwright.config.ts:3`; with `PORT=abc`,
`Number("abc")` yields `NaN`.
3. Still during config evaluation, it constructs `baseURL` as `http://127.0.0.1:${PORT}`
at `playwright.config.ts:4`, producing `http://127.0.0.1:NaN`, and prepares the dev server
command `npm run dev -- --port ${PORT}` at `playwright.config.ts:23-27`, which becomes
`npm run dev -- --port NaN`.
4. Playwright then tries to launch the Next.js dev server using this invalid command
(`--port NaN`) and wait for `url: baseURL` (`http://127.0.0.1:NaN`) as configured at
`playwright.config.ts:23-28`; Next.js and/or Playwright fails to bind or detect a server
on port `NaN`, causing the `E2E Mobile Web` workflow
(`.github/workflows/e2e-mobile.yml:1-19`) and local `npx playwright test` runs to fail
before any tests execute.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** playwright.config.ts
**Line:** 3:4
**Comment:**
*Possible Bug: `process.env.PORT` is coerced with `Number(...)` without validation, so a non-numeric value (for example from CI/env misconfiguration) becomes `NaN`. That produces an invalid `baseURL` and an invalid `--port` argument for the web server command, causing Playwright startup to fail before tests run. Validate the parsed port and fall back to a known default when parsing fails.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| try { | ||
| await target.tap(); | ||
| } catch { | ||
| await target.tap({ force: true }); |
There was a problem hiding this comment.
Suggestion: The tap helper falls back to tap({ force: true }) for any failure, which can trigger clicks on covered or non-actionable elements and make tests pass even when real users cannot tap the control. Restrict the fallback to specific transient errors (or remove forced taps) so interaction failures surface correctly. [logic error]
Severity Level: Major ⚠️
- ❌ Mobile auth and session E2E may miss non-tappable controls.
- ⚠️ CI may show green despite real interaction regressions.Steps of Reproduction ✅
1. Run the mobile Playwright tests (e.g., `npx playwright test e2e/auth/login.spec.ts`)
which import `tap` from `e2e/utils/mobile-helpers.ts` at `e2e/auth/login.spec.ts:2-7`.
2. In `e2e/auth/login.spec.ts:9-37`, the test `"login to session shell with touch and
mobile nav"` calls `await tap(enterButton);` at line 20 on the `Enter` button locator.
3. The `tap` helper implementation at `e2e/utils/mobile-helpers.ts:14-21` scrolls the
locator into view and calls `await target.tap();` at line 18; under Playwright, this
throws if the element is covered, not hittable, or otherwise non-actionable.
4. The `catch` block at `e2e/utils/mobile-helpers.ts:19-20` unconditionally retries `await
target.tap({ force: true });`, which bypasses Playwright's hit-target checks so the click
succeeds even when the control is effectively untappable for real users, allowing
downstream assertions in `login.spec.ts` and other specs (e.g., navigation headings) to
pass and masking interaction regressions.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** e2e/utils/mobile-helpers.ts
**Line:** 17:20
**Comment:**
*Logic Error: The `tap` helper falls back to `tap({ force: true })` for any failure, which can trigger clicks on covered or non-actionable elements and make tests pass even when real users cannot tap the control. Restrict the fallback to specific transient errors (or remove forced taps) so interaction failures surface correctly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const upperBox = await getRequiredBox(upper, labels.upper); | ||
| const lowerBox = await getRequiredBox(lower, labels.lower); |
There was a problem hiding this comment.
Suggestion: expectNoVerticalOverlap measures upper and lower after separate scrollIntoViewIfNeeded() calls inside getRequiredBox, so the second measurement can change scroll position and invalidate the first box coordinates. This can produce flaky or incorrect overlap assertions; capture both boxes in the same viewport state. [possible bug]
Severity Level: Major ⚠️
- ❌ Safe-area layout tests can misreport control overlap conditions.
- ⚠️ Overflow scrolling tests may become flaky with tall content.Steps of Reproduction ✅
1. Run the layout tests `e2e/layout/safe-area.spec.ts` and `e2e/layout/overflow.spec.ts`,
which import `expectNoVerticalOverlap` from `e2e/utils/mobile-helpers.ts` at
`safe-area.spec.ts:1-8` and `overflow.spec.ts:1-8`.
2. In `e2e/layout/safe-area.spec.ts:11-26`, the test `"home and nav controls stay visible
above iOS-like bottom safe area"` calls `await expectNoVerticalOverlap(beginButton,
homeNav, {...});` at line 22; similarly, `e2e/layout/overflow.spec.ts:39-45` calls `await
expectNoVerticalOverlap(lastEntry, homeNav, {...});` at line 41.
3. The `expectNoVerticalOverlap` helper at `e2e/utils/mobile-helpers.ts:58-71` computes
`upperBox` and `lowerBox` by calling `getRequiredBox` twice (lines 64-65);
`getRequiredBox` at lines 6-11 always calls `await target.scrollIntoViewIfNeeded();` and
then `await target.boundingBox();`.
4. When the two locators cannot both be fully visible without scrolling (e.g., tall
history content or future reuse with widely separated elements), the second
`scrollIntoViewIfNeeded()` for `lower` can change the viewport scroll position before its
`boundingBox()` measurement, while `upperBox` still reflects the previous viewport.
Because Playwright's `boundingBox()` coordinates are viewport-relative, `upperBottom` and
`lowerTop` in `expectNoVerticalOverlap` are then taken from different viewport states,
leading to flaky or incorrect overlap assertions in these safe-area and overflow layout
tests.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** e2e/utils/mobile-helpers.ts
**Line:** 64:65
**Comment:**
*Possible Bug: `expectNoVerticalOverlap` measures `upper` and `lower` after separate `scrollIntoViewIfNeeded()` calls inside `getRequiredBox`, so the second measurement can change scroll position and invalidate the first box coordinates. This can produce flaky or incorrect overlap assertions; capture both boxes in the same viewport state.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 7113269. Configure here.
| await page.mouse.down(); | ||
| await page.mouse.move(centerX, Math.min(viewport.height - 20, 250), { steps: 12 }); | ||
| await page.mouse.up(); | ||
| } |
There was a problem hiding this comment.
Pull-to-refresh helper uses mouse, not touch events
Low Severity
simulatePullToRefreshGesture uses page.mouse (move/down/up) to simulate a mobile touch gesture, but Playwright's page.mouse always dispatches mouse events regardless of hasTouch device configuration. All three mobile projects in playwright.config.ts use iPhone device descriptors with hasTouch: true. For a faithful touch-based overscroll simulation, page.touchscreen should be used. The current implementation dispatches mouse events that won't trigger touch-specific browser behaviors like native overscroll or pull-to-refresh interception, making the test less representative of actual mobile user interaction than its name implies.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7113269. Configure here.
|
Closing as superseded by #225, which was merged on 2026-04-24 with the same title and equivalent (in fact strictly broader) coverage. Verified that current main contains everything in this PR:
Rebasing this branch onto main would either replay #225's work as a no-op or regress #307's fixture fixes. No unique content to salvage. |
2 participants
User description
Summary
playwright.config.ts) with three mobile projects:mobile-chromium-narrow(iPhone SE)mobile-chromium-wide(iPhone 13 Pro Max)mobile-webkit-iphone(iPhone 13 / Safari-class behavior)e2e/utils/mobile-helpers.tsfor tap interactions, tap-target sizing, overflow checks, viewport/safe-area visibility assertions, and overlap checkse2e/auth/login.spec.tse2e/session/session-flow.spec.tse2e/layout/safe-area.spec.tse2e/layout/overflow.spec.tse2e/fixtures/auth.fixture.tsso mobile E2E scenarios run consistently in CI.github/workflows/e2e-mobile.ymlto run mobile matrix jobs (Chromium narrow + WebKit iPhone)package.jsonand ignore Playwright artifact directories in.gitignoresrc/components/SessionView.tsxsrc/components/CompletionScreen.tsxsrc/app/app/page.tsxAcceptance Criteria Mapping
tap()and bottom nav transitions ine2e/auth/login.spec.tse2e/session/session-flow.spec.tse2e/layout/safe-area.spec.tse2e/layout/overflow.spec.tse2e/layout/overflow.spec.tse2e/session/session-flow.spec.tse2e/session/session-flow.spec.tse2e/layout/overflow.spec.tse2e/auth/login.spec.tse2e/auth/login.spec.tsCI Matrix Tradeoff Note
This change exceeds the minimum by including two mobile Chromium widths plus a WebKit iPhone project. The workflow currently runs narrow Chromium + WebKit as matrix jobs, while wide Chromium remains available as a local/optional CI target via project selection.
Validation
npm run test:e2e -- --project mobile-chromium-narrownpm run test:e2e -- --project mobile-chromium-widenpm run test:e2e -- --project mobile-webkit-iphoneNote on CodeRabbit CLI checkpoint
Attempted
coderabbit review --prompt-onlyas requested, but CLI auth is required in this environment (Authentication required. Please run 'coderabbit auth login' or provide --api-key).CodeAnt-AI Description
Add mobile web test coverage for login, session flow, and layout safety
What Changed
Impact
✅ Fewer mobile login and session flow breaks✅ Fewer buttons hidden under browser chrome✅ Easier tapping on phone-sized screens🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.