[STU-175] Add responsive layout smoke tests for mobile and tablet viewports#40
Conversation
…ts [STU-175] 20 tests covering 3 pages (landing, login, candidate portal) across mobile (390x844) and tablet (768x1024) viewports with assertions for horizontal overflow, readable text, tap target sizing, and navigation adaptation (mobile tab bar vs sidebar). Also adds a tablet project (768x1024, 2x scale) to Playwright config. Tests: added responsive smoke test suite (e2e/smoke/responsive.spec.ts) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR adds comprehensive responsive smoke testing to the Playwright suite. It introduces a new tablet device configuration (768×1024), creates reusable validation helpers for detecting overflow and tap-target issues, and implements parallel test suites covering mobile and tablet viewports for unauthenticated and authenticated pages. ChangesResponsive Smoke Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/smoke/responsive.spec.ts`:
- Line 2: Replace the brittle relative import in responsive.spec.ts by using the
project path alias: change the import that currently brings in getFixtures and
disconnectPrisma from "../fixtures/auth" to use the "`@/fixtures/auth`" alias so
internal fixture imports follow the TypeScript import-path convention and remain
stable across file moves; update the import statement that references
getFixtures and disconnectPrisma accordingly.
- Around line 290-307: The test "candidate portal shows floating tab bar, hides
sidebar" currently accepts either nav being visible; change the assertions to
explicitly require the mobile tab bar to be visible and the workspace
rail/sidebar to be hidden: use the existing locators tabBar (".mobileTabBar")
and rail (".workspaceRail") and replace the combined expect with two explicit
checks—assert tabBar is visible (e.g., expect(tabBar).toBeVisible with an
appropriate timeout) and assert rail is not visible/hidden (e.g.,
expect(rail).toBeHidden or verify rail.isVisible() resolves false) so the test
fails if both are visible or the sidebar is shown. Ensure you keep
createAuthedPage and tabletViewport setup and preserve error handling for
locator visibility checks.
- Around line 121-124: The test hard-codes the auth cookie domain to "127.0.0.1"
(the cookie named "studenthub_next_session" using candidate.cookie), which
breaks when PLAYWRIGHT_BASE_URL points to a different host; change the
cookie-setting logic to derive the domain from the test's base URL (e.g. use
PLAYWRIGHT_BASE_URL or the page/context URL) or omit the domain so the cookie
applies to the current host instead of always using "127.0.0.1".
🪄 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 Plus
Run ID: 1c3934e3-842e-4b8e-a569-f9391dfc2ba2
📒 Files selected for processing (2)
e2e/smoke/responsive.spec.tsplaywright.config.ts
| @@ -0,0 +1,340 @@ | |||
| import { test, expect, type Page, type Browser } from "@playwright/test"; | |||
| import { getFixtures, disconnectPrisma } from "../fixtures/auth"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use the @/ alias for internal fixture imports.
This relative import violates the TypeScript import-path convention and is brittle on file moves.
Suggested change
-import { getFixtures, disconnectPrisma } from "../fixtures/auth";
+import { getFixtures, disconnectPrisma } from "`@/e2e/fixtures/auth`";As per coding guidelines, "Use @/ path alias for all internal imports in TypeScript files".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { getFixtures, disconnectPrisma } from "../fixtures/auth"; | |
| import { getFixtures, disconnectPrisma } from "`@/e2e/fixtures/auth`"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/smoke/responsive.spec.ts` at line 2, Replace the brittle relative import
in responsive.spec.ts by using the project path alias: change the import that
currently brings in getFixtures and disconnectPrisma from "../fixtures/auth" to
use the "`@/fixtures/auth`" alias so internal fixture imports follow the
TypeScript import-path convention and remain stable across file moves; update
the import statement that references getFixtures and disconnectPrisma
accordingly.
| name: "studenthub_next_session", | ||
| value: candidate.cookie, | ||
| domain: "127.0.0.1", | ||
| path: "/", |
There was a problem hiding this comment.
Avoid hard-coding the auth cookie domain.
Line 123 binds auth to 127.0.0.1, so authenticated tests can fail when PLAYWRIGHT_BASE_URL uses another host.
Suggested change
{
name: "studenthub_next_session",
value: candidate.cookie,
- domain: "127.0.0.1",
- path: "/",
+ url: process.env.PLAYWRIGHT_BASE_URL ?? "http://127.0.0.1:3000",
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: "studenthub_next_session", | |
| value: candidate.cookie, | |
| domain: "127.0.0.1", | |
| path: "/", | |
| name: "studenthub_next_session", | |
| value: candidate.cookie, | |
| url: process.env.PLAYWRIGHT_BASE_URL ?? "http://127.0.0.1:3000", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/smoke/responsive.spec.ts` around lines 121 - 124, The test hard-codes the
auth cookie domain to "127.0.0.1" (the cookie named "studenthub_next_session"
using candidate.cookie), which breaks when PLAYWRIGHT_BASE_URL points to a
different host; change the cookie-setting logic to derive the domain from the
test's base URL (e.g. use PLAYWRIGHT_BASE_URL or the page/context URL) or omit
the domain so the cookie applies to the current host instead of always using
"127.0.0.1".
| test("candidate portal shows floating tab bar, hides sidebar", async ({ browser }) => { | ||
| const { page, context } = await createAuthedPage(browser, tabletViewport); | ||
| await page.goto("/candidate"); | ||
| await expect(page.locator('text="Readiness"')).toBeVisible({ timeout: 15000 }); | ||
|
|
||
| // At 768px, the mobileTabBar should be visible (full-width bottom bar) | ||
| // and workspaceRail hidden (max-width: 768px rule triggers display:none) | ||
| const tabBar = page.locator(".mobileTabBar").first(); | ||
| const tabBarVisible = await tabBar.isVisible().catch(() => false); | ||
|
|
||
| const rail = page.locator(".workspaceRail").first(); | ||
| const railVisible = await rail.isVisible().catch(() => false); | ||
|
|
||
| // At least one nav element must be visible | ||
| expect( | ||
| tabBarVisible || railVisible, | ||
| "expected either mobileTabBar or workspaceRail to be visible", | ||
| ).toBe(true); |
There was a problem hiding this comment.
Tablet nav assertion does not validate the stated behavior.
The test says sidebar is hidden, but it currently passes if either nav is visible. A regression where both are visible would not fail.
Suggested change
- const tabBarVisible = await tabBar.isVisible().catch(() => false);
-
const rail = page.locator(".workspaceRail").first();
- const railVisible = await rail.isVisible().catch(() => false);
-
- // At least one nav element must be visible
- expect(
- tabBarVisible || railVisible,
- "expected either mobileTabBar or workspaceRail to be visible",
- ).toBe(true);
+ await expect(tabBar).toBeVisible();
+ await expect(rail).not.toBeVisible();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/smoke/responsive.spec.ts` around lines 290 - 307, The test "candidate
portal shows floating tab bar, hides sidebar" currently accepts either nav being
visible; change the assertions to explicitly require the mobile tab bar to be
visible and the workspace rail/sidebar to be hidden: use the existing locators
tabBar (".mobileTabBar") and rail (".workspaceRail") and replace the combined
expect with two explicit checks—assert tabBar is visible (e.g.,
expect(tabBar).toBeVisible with an appropriate timeout) and assert rail is not
visible/hidden (e.g., expect(rail).toBeHidden or verify rail.isVisible()
resolves false) so the test fails if both are visible or the sidebar is shown.
Ensure you keep createAuthedPage and tabletViewport setup and preserve error
handling for locator visibility checks.
BAWES
left a comment
BAWES
left a comment
There was a problem hiding this comment.
Staff Engineer Review — APPROVE WITH NOTES
Solid smoke test suite. 20 tests covering 3 pages at 2 breakpoints, with meaningful assertions (overflow, readable text, tap targets, navigation adaptation). Good addition to the test pyramid.
What's good
- Test helpers (
assertNoHorizontalOverflow,assertTextReadable,assertTapTargets) are reusable and well-scoped test.describe.configure({ mode: "serial" })correctly used for candidate portal tests that share the authed fixturedisconnectPrisma()intest.afterAllprevents connection leaks- Tablet project added cleanly to Playwright config
CodeRabbit findings — disposition
-
Import path
../fixtures/auth— CodeRabbit suggested@/fixtures/auth. This is incorrect. The@/path alias maps tosrc/(per tsconfig), while the fixtures live ate2e/fixtures/auth.ts. The relative import../fixtures/authfrome2e/smoke/responsive.spec.tsis the correct path. No change needed. -
Hardcoded cookie domain
127.0.0.1— Valid observation, but the project'sbaseURL(playwright.config.ts line 11) defaults tohttp://127.0.0.1:3000, so the two are consistent. If someone overridesPLAYWRIGHT_BASE_URLto a different host, the cookie domain would need to match. Low severity; acceptable for now. -
Tablet test relaxed assertion — The test at line 290-307 uses
expect(tabBarVisible || railVisible)because 768px is the exact breakpoint boundary where CSS@media (max-width: 768px)behavior can vary by browser engine. The relaxed assertion is a pragmatic choice that avoids flaky tests at the boundary. Acceptable.
Verification
- CI: all green ✓
- Branch naming:
feature/STU-175-*✓ - Commit: conventional ✓
1 participant
Summary
/), login (/login), and candidate portal (/candidate) pagestabletproject to Playwright config (768×1024, 2x device scale)Test plan
chromiumprojectnpm run test:validate— 76/76 still pass🤖 Generated with Claude Code
Summary by CodeRabbit
Tests