Add mobile web E2E coverage for viewport, touch, safe-area, and overflow#225
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:
📝 WalkthroughWalkthroughAdds Playwright-based mobile web E2E: CI workflow, Playwright config and device projects, fixtures with an in-memory mock API, mobile UI helpers and tests, npm scripts/devDependency, .gitignore updates, and minor UI style tweaks for tappability and safe-area constraints. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Playwright Test
participant Browser as Mobile Browser (Playwright)
participant App as Dev Server / App
participant Mock as Mock API (route handlers)
Test->>Browser: launch with device profile, navigate to /app
Browser->>App: GET /app (dev server or PLAYWRIGHT_BASE_URL)
Browser->>Mock: intercept /api/** requests
Mock-->>Browser: respond with mocked auth/sessions/thoughts
Test->>Browser: perform taps/scrolls/pull-to-refresh
Browser->>Mock: POST/GET /api endpoints (session/thoughts)
Mock-->>Browser: persist and return updated mock state
Test->>Test: assert viewport/tap targets/no overflow/visibility
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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.
| matrix: | ||
| project: | ||
| - mobile-chromium-narrow | ||
| - mobile-webkit-iphone |
There was a problem hiding this comment.
CI matrix omits the mobile-chromium-wide project
Low Severity
The CI matrix only includes mobile-chromium-narrow and mobile-webkit-iphone, but playwright.config.ts defines a third project, mobile-chromium-wide (iPhone 13 Pro Max dimensions), which is also listed in the PR's own validation section. This means wider-viewport layout regressions (e.g. flex overflow, element stretching) won't be caught in CI, even though the project is fully configured and intended for use.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7113269. Configure here.
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: 2
🧹 Nitpick comments (3)
src/components/SessionView.tsx (1)
485-485: Make action-row tap targets deterministic with explicitminHeight.At Line 485, Line 503, Line 521, and Line 539, padding-only sizing can still render slightly under 44px depending on font metrics. Add
minHeight: "44px"to guarantee target size across engines.♻️ Suggested change
style={{ background: "none", border: "1px solid var(--border-2)", color: "var(--fg-3)", ...mono, fontSize: "11px", letterSpacing: "0.15em", textTransform: "uppercase", padding: "14px 24px", + minHeight: "44px", borderRadius: "20px", cursor: "pointer", }} style={{ background: "none", border: "1px solid var(--accent-amber-border)", color: "var(--accent-amber-border)", ...mono, fontSize: "11px", letterSpacing: "0.15em", textTransform: "uppercase", padding: "14px 24px", + minHeight: "44px", borderRadius: "20px", cursor: "pointer", }} style={{ background: "none", border: "1px solid var(--accent-green-border)", color: "var(--accent-green-dim)", ...mono, fontSize: "11px", letterSpacing: "0.15em", textTransform: "uppercase", padding: "14px 24px", + minHeight: "44px", borderRadius: "20px", cursor: "pointer", }} style={{ background: "none", border: "1px solid var(--accent-danger-border)", color: "var(--accent-danger-muted)", ...mono, fontSize: "11px", letterSpacing: "0.15em", textTransform: "uppercase", padding: "14px 24px", + minHeight: "44px", borderRadius: "20px", cursor: "pointer", }}Also applies to: 503-503, 521-521, 539-539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SessionView.tsx` at line 485, In SessionView.tsx, the action-row style entries use padding-only sizing which can render below 44px; update each action-row style object (the inline style in the SessionView component where padding: "14px 24px" appears) to include minHeight: "44px" so the tap targets are deterministic; apply this change to the four occurrences matching the action-row padding at the locations in the component (the four inline style objects that currently set padding: "14px 24px")..github/workflows/e2e-mobile.yml (1)
39-45: Uploadtest-resultsartifacts too for better failure diagnostics.Current upload captures only the HTML report. Including
test-results/preserves traces/screenshots/videos for flaky mobile failures.🧪 Suggested workflow tweak
- name: Upload Playwright report if: always() uses: actions/upload-artifact@v4 with: name: playwright-report-${{ matrix.project }} - path: playwright-report/ + path: | + playwright-report/ + test-results/ if-no-files-found: ignore🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-mobile.yml around lines 39 - 45, Add an additional artifact upload step (using actions/upload-artifact@v4) to also upload the test-results directory so traces/screenshots/videos are preserved; keep the same if: always() condition and the matrix.project naming pattern (e.g., name: playwright-results-${{ matrix.project }} and path: test-results/) and set if-no-files-found: ignore to avoid failing when artifacts are absent; place this step near the existing "Upload Playwright report" step so both playwright-report/ and test-results/ are uploaded for failure diagnostics.e2e/utils/mobile-helpers.ts (1)
74-82: This helper is a mouse drag, not a touch gesture.Lines 78-81 only send mouse events, so the “pull to refresh” path is not actually validating touch-specific behavior. If the app’s overscroll handling depends on touch/pointer input, this can be a silent no-op while the test still passes. Either drive a touch-capable gesture path here or rename the helper so the coverage claim stays accurate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/utils/mobile-helpers.ts` around lines 74 - 82, The helper simulatePullToRefreshGesture currently uses only mouse events (page.mouse.move/down/up) which don't exercise touch-specific code; replace the mouse-driven sequence with touch events by using Playwright's touch input (e.g., input.dispatchTouchEvent or page.touchscreen APIs) to emit touchStart/touchMove/touchEnd gestures at the same coordinates (use the computed centerX and target Y) so overscroll/touch handlers run, or if you intentionally want a mouse drag, rename simulatePullToRefreshGesture to something like simulateMouseDragToRefresh and keep the current implementation; update any callers/tests accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/fixtures/auth.fixture.ts`:
- Around line 180-182: Mocked response returns state.thoughts in insertion
order; change the GET /api/thoughts branch in auth.fixture.ts to return a sorted
copy of state.thoughts (don’t mutate original) using the same ordering as
src/app/api/thoughts/route.ts: sort by descending dayNumber (higher dayNumber
first) and for equal dayNumber sort by ascending timeInSession (lower
timeInSession first) before passing to json(route, 200, ...).
In `@e2e/utils/mobile-helpers.ts`:
- Around line 14-21: In the tap helper function, remove the force-tap fallback
so Playwright's actionability checks are preserved: replace the try/catch that
calls target.tap({ force: true }) with a single awaited call to target.tap()
(after target.scrollIntoViewIfNeeded() and expect(target).toBeVisible()) and let
any thrown error propagate (do not swallow or force the action); this ensures
the tap helper (function tap / Locator target usage) surfaces
layout/actionability failures instead of bypassing them.
---
Nitpick comments:
In @.github/workflows/e2e-mobile.yml:
- Around line 39-45: Add an additional artifact upload step (using
actions/upload-artifact@v4) to also upload the test-results directory so
traces/screenshots/videos are preserved; keep the same if: always() condition
and the matrix.project naming pattern (e.g., name: playwright-results-${{
matrix.project }} and path: test-results/) and set if-no-files-found: ignore to
avoid failing when artifacts are absent; place this step near the existing
"Upload Playwright report" step so both playwright-report/ and test-results/ are
uploaded for failure diagnostics.
In `@e2e/utils/mobile-helpers.ts`:
- Around line 74-82: The helper simulatePullToRefreshGesture currently uses only
mouse events (page.mouse.move/down/up) which don't exercise touch-specific code;
replace the mouse-driven sequence with touch events by using Playwright's touch
input (e.g., input.dispatchTouchEvent or page.touchscreen APIs) to emit
touchStart/touchMove/touchEnd gestures at the same coordinates (use the computed
centerX and target Y) so overscroll/touch handlers run, or if you intentionally
want a mouse drag, rename simulatePullToRefreshGesture to something like
simulateMouseDragToRefresh and keep the current implementation; update any
callers/tests accordingly.
In `@src/components/SessionView.tsx`:
- Line 485: In SessionView.tsx, the action-row style entries use padding-only
sizing which can render below 44px; update each action-row style object (the
inline style in the SessionView component where padding: "14px 24px" appears) to
include minHeight: "44px" so the tap targets are deterministic; apply this
change to the four occurrences matching the action-row padding at the locations
in the component (the four inline style objects that currently set padding:
"14px 24px").
🪄 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: d20f2caf-e394-4050-9b21-ca0123c85cae
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.github/workflows/e2e-mobile.yml.gitignoree2e/auth/login.spec.tse2e/fixtures/auth.fixture.tse2e/layout/overflow.spec.tse2e/layout/safe-area.spec.tse2e/session/session-flow.spec.tse2e/utils/mobile-helpers.tspackage.jsonplaywright.config.tssrc/app/app/page.tsxsrc/components/CompletionScreen.tsxsrc/components/SessionView.tsx
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e/layout/safe-area.spec.ts (1)
11-24: ExtracttapWithControlRevealinto shared mobile helpersThis helper is duplicated here and in
e2e/session/session-flow.spec.ts(Lines 10-23). Centralizing it avoids drift and keeps retry behavior consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/layout/safe-area.spec.ts` around lines 11 - 24, Extract the duplicated tapWithControlReveal function into a single shared mobile helper module and update both specs to import it: create a new exported function with the same signature and retry behavior as the current tapWithControlReveal (preserve the Page and Locator params, three-attempt loop, touchstart dispatch, tap call, waitForTimeout, and rethrow lastError), replace the duplicate implementations in e2e/layout/safe-area.spec.ts and e2e/session/session-flow.spec.ts with an import of the new helper, and ensure the helper is exported so tests can import it and any required Playwright types (Page, Locator) are referenced or re-exported as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/layout/safe-area.spec.ts`:
- Around line 30-37: The Home nav button (homeNav created via page.getByRole) is
missing a viewport/safe-area bounds check; add a call to the same viewport
assertion used for beginButton by invoking expectVisibleInViewport(page,
homeNav, "home nav button") alongside the existing toBeVisible() and
expectMinimumTapTarget(homeNav, "home nav button") so the nav control is also
validated against clipping at viewport/safe-area edges.
In `@e2e/session/session-flow.spec.ts`:
- Around line 1-10: The function tapWithControlReveal uses invalid type
extraction (Parameters<typeof test>[0]["page"] and Parameters<typeof tap>[0]);
replace these with explicit Playwright types by importing Page and Locator
(e.g., import type { Page, Locator } from "@playwright/test") and change the
signature to async function tapWithControlReveal(page: Page, target: Locator) so
the parameters have correct, direct types; locate the tapWithControlReveal
declaration to update the signature and add the type import near the top of the
file.
---
Nitpick comments:
In `@e2e/layout/safe-area.spec.ts`:
- Around line 11-24: Extract the duplicated tapWithControlReveal function into a
single shared mobile helper module and update both specs to import it: create a
new exported function with the same signature and retry behavior as the current
tapWithControlReveal (preserve the Page and Locator params, three-attempt loop,
touchstart dispatch, tap call, waitForTimeout, and rethrow lastError), replace
the duplicate implementations in e2e/layout/safe-area.spec.ts and
e2e/session/session-flow.spec.ts with an import of the new helper, and ensure
the helper is exported so tests can import it and any required Playwright types
(Page, Locator) are referenced or re-exported as needed.
🪄 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: 1fcf0366-e9ec-46df-8979-74f890810e01
📒 Files selected for processing (2)
e2e/layout/safe-area.spec.tse2e/session/session-flow.spec.ts
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/utils/mobile-helpers.ts (1)
20-31: Extract retry magic numbers into named constants.
3attempts and120msdelay are reasonable, but naming them makes tuning and intent clearer across specs.♻️ Suggested cleanup
export const MOBILE_SAFE_AREA_TOLERANCE_PX = 6; export const MIN_TAP_TARGET_PX = 44; +const CONTROL_REVEAL_TAP_RETRIES = 3; +const CONTROL_REVEAL_RETRY_DELAY_MS = 120; export async function tapWithControlReveal(page: Page, target: Locator) { let lastError: unknown = null; - for (let attempt = 0; attempt < 3; attempt += 1) { + for (let attempt = 0; attempt < CONTROL_REVEAL_TAP_RETRIES; attempt += 1) { await page.dispatchEvent("body", "touchstart"); try { await tap(target); return; } catch (error) { lastError = error; - await page.waitForTimeout(120); + await page.waitForTimeout(CONTROL_REVEAL_RETRY_DELAY_MS); } } throw lastError; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/utils/mobile-helpers.ts` around lines 20 - 31, Extract the magic numbers in tapWithControlReveal into named constants (e.g., MAX_TAP_ATTEMPTS and TAP_RETRY_DELAY_MS) defined at file scope and replace the literal 3 and 120 with those constants; update the for-loop to use MAX_TAP_ATTEMPTS and pass TAP_RETRY_DELAY_MS into page.waitForTimeout to make retry behavior explicit and easy to tune.e2e/session/session-flow.spec.ts (1)
51-55: Remove redundant pre-reveal touch dispatch before helper call.
tapWithControlReveal()already performsbodytouchstartinternally, so the extra pre-dispatches here are duplicate behavior.🧹 Suggested simplification
- await page.dispatchEvent("body", "touchstart"); - await expect(page.getByRole("button", { name: /end early/i })).toBeVisible(); await tapWithControlReveal(page, page.getByRole("button", { name: /end early/i })); @@ - await page.dispatchEvent("body", "touchstart"); await expectVisibleInViewport(page, endEarlyButton, "landscape end early button"); await tapWithControlReveal(page, endEarlyButton);Also applies to: 69-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/session/session-flow.spec.ts` around lines 51 - 55, The test currently dispatches a redundant 'touchstart' on body immediately before calling tapWithControlReveal() (which already performs that touchstart); remove the explicit page.dispatchEvent("body", "touchstart") calls (and any duplicate pre-touchstart in the other occurrence) so the flow relies on tapWithControlReveal(page, ...) to perform the reveal, keeping the surrounding expects and button selectors (tapWithControlReveal and the getByRole assertions) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/session/session-flow.spec.ts`:
- Around line 51-55: The test currently dispatches a redundant 'touchstart' on
body immediately before calling tapWithControlReveal() (which already performs
that touchstart); remove the explicit page.dispatchEvent("body", "touchstart")
calls (and any duplicate pre-touchstart in the other occurrence) so the flow
relies on tapWithControlReveal(page, ...) to perform the reveal, keeping the
surrounding expects and button selectors (tapWithControlReveal and the getByRole
assertions) unchanged.
In `@e2e/utils/mobile-helpers.ts`:
- Around line 20-31: Extract the magic numbers in tapWithControlReveal into
named constants (e.g., MAX_TAP_ATTEMPTS and TAP_RETRY_DELAY_MS) defined at file
scope and replace the literal 3 and 120 with those constants; update the
for-loop to use MAX_TAP_ATTEMPTS and pass TAP_RETRY_DELAY_MS into
page.waitForTimeout to make retry behavior explicit and easy to tune.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bd298d8-a034-4488-9413-b534ff46ebf1
📒 Files selected for processing (4)
e2e/layout/overflow.spec.tse2e/layout/safe-area.spec.tse2e/session/session-flow.spec.tse2e/utils/mobile-helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/layout/overflow.spec.ts
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
13 tasks
This was referenced Apr 25, 2026
Merged
This was referenced May 1, 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
playwright.config.ts) with mobile Chromium + WebKit projects.github/workflows/e2e-mobile.ymlmobile-chromium-widein CI matrix (narrow + wide Chromium + WebKit)/api/thoughtsordering with API route behaviortaphelperminHeight: "44px"to session action-row controlstest-results/artifacts in CImobile-chromium-wideCI failures by introducing explicit retry/touch-reveal tap helper usage in the two affected session-path specsValidation
npm run test:e2e -- --project mobile-chromium-narrownpm run test:e2e -- --project mobile-webkit-iphonenpm run test:e2e -- --project mobile-chromium-wideSummary by CodeRabbit
Chores
Tests
Bug Fixes / Accessibility
Ignore rules