fix(a11y+e2e): landing/signup contrast (real) + auth spec drift — latent failures unmasked by #940 - #941
Conversation
…th specs
The main e2e suite (accessibility.spec.ts + auth.spec.ts) had been SKIPPED for
a while: the "Run Playwright tests" step in playwright.yml runs only on success,
and it sat behind the blocking "mandatory smoke + mobile viewport regression"
step, which was red on the merge-train base. Fixing that blocking step (this
branch) let the main suite run for the first time in this window, surfacing four
PRE-EXISTING latent failures — none introduced by the branch delta.
a11y (real product bugs — WCAG 2 AA color-contrast):
- Landing "Request Demo" and signup gate "Continue" buttons rendered white text
on primary-600 (#16a34a) = 3.29:1 (< 4.5:1). The login pages share the same
button color but PASS because their submit is gated behind an async
`checkingAuth` render-gate, so axe never sees it; these two buttons render
synchronously. Bumped both to primary-700 (#15803d, ~5.0:1), hover/active
darken to stay compliant. Stays in the primary green family.
- Landing's decorative fake-browser mockup had a dim URL bar (#565656 on
#0e0e0e = 2.63:1). Marked the mockup `inert` — correct semantics for
non-interactive decoration, and axe's color-contrast matcher exempts inert
subtrees.
auth (spec drift — product behavior is correct):
- "log in as a coach": login succeeds and reaches /baseball/dashboard/
command-center; the final `locator('text=Dashboard')` now matches 3 nodes
(sidebar link + breadcrumb current + Fairway breadcrumb transition span) and
throws a strict-mode violation. Target the sidebar nav link specifically.
- "redirect ... while logged out": middleware correctly redirects to
/baseball/login?returnTo=... (long-standing behavior); the bare `**/login`
glob can't match a URL with a query string. Use a query-tolerant regex.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Summary by CodeRabbit
WalkthroughBaseball E2E tests now authenticate with pinned fixture accounts and use deterministic route/game assertions. Authentication checks tolerate current dashboard and redirect behavior. Signup and landing-page controls use darker colors, and the decorative browser frame is marked inert. ChangesSeeded E2E reliability
UI contrast and accessibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BaseballE2ETest
participant FixtureAuthHelper
participant BaseballLogin
participant SeededBaseballData
BaseballE2ETest->>FixtureAuthHelper: authenticate with fixture account
FixtureAuthHelper->>BaseballLogin: submit pinned credentials
BaseballLogin-->>FixtureAuthHelper: return authenticated route
FixtureAuthHelper-->>BaseballE2ETest: continue test setup
BaseballE2ETest->>SeededBaseballData: locate seeded game or camp
SeededBaseballData-->>BaseballE2ETest: provide deterministic fixture content
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)ast-grep could not parse rule config: /ast-grep-rules/../git/.coderabbit/ast-grep/no-explicit-any.yml 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/auth.spec.ts`:
- Around line 35-37: Update the Dashboard visibility assertion in the
authentication test to remove the .first() call and retain Playwright’s strict
locator behavior; if multiple matching links remain, scope getByRole to the
appropriate navigation landmark instead of selecting the first match.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 9d896388-1f53-41c1-9c4d-b8074bc0fa45
📒 Files selected for processing (3)
e2e/auth.spec.tssrc/app/golf/(auth)/signup/page.tsxsrc/components/landing/Hero.tsx
| await expect(page.getByRole('link', { name: 'Dashboard' }).first()).toBeVisible({ | ||
| timeout: 5000, | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove .first() to preserve Playwright strict mode.
e2e/auth.spec.ts:35. Since the other two conflicting nodes are span elements, getByRole('link', { name: 'Dashboard' }) will strictly match only the link. Using .first() disables strict mode, which would otherwise catch future regressions if duplicate links are accidentally introduced. If multiple links do exist, scope the locator to its specific navigation landmark instead of bypassing strictness.
♻️ Proposed fix
- await expect(page.getByRole('link', { name: 'Dashboard' }).first()).toBeVisible({
+ await expect(page.getByRole('link', { name: 'Dashboard' })).toBeVisible({
timeout: 5000,
});📝 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.
| await expect(page.getByRole('link', { name: 'Dashboard' }).first()).toBeVisible({ | |
| timeout: 5000, | |
| }); | |
| await expect(page.getByRole('link', { name: 'Dashboard' })).toBeVisible({ | |
| timeout: 5000, | |
| }); |
🤖 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/auth.spec.ts` around lines 35 - 37, Update the Dashboard visibility
assertion in the authentication test to remove the .first() call and retain
Playwright’s strict locator behavior; if multiple matching links remain, scope
getByRole to the appropriate navigation landmark instead of selecting the first
match.
…ts-smoke asserts The chromium e2e project ran end-to-end for the first time (playwright.yml step was previously masked by earlier-step failures). A layer of baseball specs failed deterministically. Root cause for the largest cluster: the #375-fixture specs (baseball-box-score, baseball-pipeline, camps) log in via the shared env-driven loginAsCoach/loginAsPlayer, which the seeded-smoke lane (#382) repoints at the Rini demo coach through E2E_BASEBALL_COACH_EMAIL. But their fixtures are seeded by scripts/seed-baseball-e2e.ts onto a SEPARATE team ("E2E Test University Baseball", owner testcoach@helm.test). So the specs logged into the Rini demo team and saw an empty/foreign team — pipeline board rendered its "The board is open" empty state (no Jordan Hayes), seeded games were absent, seeded camp invisible. - helpers/auth.ts: add credential-pinned loginAsFixtureCoach/Player + E2E_FIXTURE_USERS (testcoach@helm.test / testplayer@helm.test), kept in sync with scripts/seed-baseball-e2e.ts, decoupled from the env-driven TEST_USERS. - baseball-box-score / baseball-pipeline / camps: log in via the fixture helpers so they exercise the team their #375 fixtures actually live on. Also de-drift two seeded-smoke assertions (these use the correct Rini demo coach; the surfaces evolved): - anon /baseball/player/today now redirects to the auth wall (the teamless terminal is reserved for an authenticated-but-teamless player per the page's HONESTY v12 docstring); assert the redirect instead of a 200 terminal. - games list: the seed produces multiple past games and two share the opponent "Coastal State", so the name-based link locator tripped strict mode — target the manifest's specific completed game by its deterministic href instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/baseball-stats-smoke.spec.ts`:
- Around line 184-186: Update the seededGameLink locator to remove the
data-testid selector and rely on the unique href attribute for the anchor
element. Preserve the existing completed-game URL matching while using the
accessible link role or href-based locator as appropriate.
- Around line 96-99: Replace the synchronous page.url/onAuthWall check in the
anonymous /baseball/player/today redirect test with Playwright’s retrying
expect(page).toHaveURL assertion. Match the allowed login, signup, or baseball
login destinations while preserving the existing redirect expectation.
In `@e2e/camps.spec.ts`:
- Around line 2-7: Remove the aliases from the fixture helper import in
e2e/camps.spec.ts, and update the test body calls from loginAsCoach and
loginAsPlayer to loginAsFixtureCoach and loginAsFixturePlayer respectively. Keep
the fixture-based login behavior unchanged.
In `@e2e/helpers/auth.ts`:
- Around line 93-111: Extract the shared navigation, credential form filling,
and submit steps from loginAsCoach/loginAsPlayer and the fixture variants into
an internal login helper that accepts the page and user credentials. Update
loginAsFixtureCoach and loginAsFixturePlayer to reuse that helper while
preserving their existing post-login URL waits and role-specific behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: d2fa98df-988c-49a7-ace2-c6904a1678a6
📒 Files selected for processing (5)
e2e/baseball-box-score.spec.tse2e/baseball-pipeline.spec.tse2e/baseball-stats-smoke.spec.tse2e/camps.spec.tse2e/helpers/auth.ts
| const url = page.url(); | ||
| const onAuthWall = | ||
| url.includes('/login') || url.includes('/signup') || url.includes('/baseball/login'); | ||
| expect(onAuthWall, 'expected anonymous /baseball/player/today to redirect to login').toBe(true); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Use web-first assertions for URL checks.
e2e/baseball-stats-smoke.spec.ts:96. page.url() and boolean assertions evaluate synchronously and do not auto-retry, increasing the risk of test flakiness on delayed client-side redirects. Use Playwright's web-first toHaveURL matcher.
🛠️ Proposed refactor
- const url = page.url();
- const onAuthWall =
- url.includes('/login') || url.includes('/signup') || url.includes('/baseball/login');
- expect(onAuthWall, 'expected anonymous /baseball/player/today to redirect to login').toBe(true);
+ await expect(page, 'expected anonymous /baseball/player/today to redirect to login').toHaveURL(
+ /.*(\/login|\/signup|\/baseball\/login)/
+ );📝 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.
| const url = page.url(); | |
| const onAuthWall = | |
| url.includes('/login') || url.includes('/signup') || url.includes('/baseball/login'); | |
| expect(onAuthWall, 'expected anonymous /baseball/player/today to redirect to login').toBe(true); | |
| await expect(page, 'expected anonymous /baseball/player/today to redirect to login').toHaveURL( | |
| /.*(\/login|\/signup|\/baseball\/login)/ | |
| ); |
🤖 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/baseball-stats-smoke.spec.ts` around lines 96 - 99, Replace the
synchronous page.url/onAuthWall check in the anonymous /baseball/player/today
redirect test with Playwright’s retrying expect(page).toHaveURL assertion. Match
the allowed login, signup, or baseball login destinations while preserving the
existing redirect expectation.
| const seededGameLink = page.locator( | ||
| `a[data-testid="game-card"][href$="/baseball/dashboard/stats/games/${BASEBALL_SEED_MANIFEST.completedGame.id}"]`, | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove data-testid when an accessible role exists.
e2e/baseball-stats-smoke.spec.ts:184. The target element is an <a> tag, which inherently provides the accessible link role. Using data-testid here directly violates the path instructions. If Playwright's strict mode prevents the use of getByRole('link', { name: ... }) due to duplicate opponent names, rely entirely on the unique href attribute. Treat accessibility and test accessibility standard issues as blocking.
As per path instructions, use data-testid only when no accessible role exists.
🛠️ Proposed fix
const seededGameLink = page.locator(
- `a[data-testid="game-card"][href$="/baseball/dashboard/stats/games/${BASEBALL_SEED_MANIFEST.completedGame.id}"]`,
+ `a[href$="/baseball/dashboard/stats/games/${BASEBALL_SEED_MANIFEST.completedGame.id}"]`,
);📝 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.
| const seededGameLink = page.locator( | |
| `a[data-testid="game-card"][href$="/baseball/dashboard/stats/games/${BASEBALL_SEED_MANIFEST.completedGame.id}"]`, | |
| ); | |
| const seededGameLink = page.locator( | |
| `a[href$="/baseball/dashboard/stats/games/${BASEBALL_SEED_MANIFEST.completedGame.id}"]`, | |
| ); |
🤖 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/baseball-stats-smoke.spec.ts` around lines 184 - 186, Update the
seededGameLink locator to remove the data-testid selector and rely on the unique
href attribute for the anchor element. Preserve the existing completed-game URL
matching while using the accessible link role or href-based locator as
appropriate.
Source: Path instructions
| // The seeded "E2E Prospect Camp" is owned by the #375 fixture coach | ||
| // (testcoach@helm.test) on the dedicated "E2E Test University" team — log in | ||
| // as those pinned accounts, NOT the env-driven Rini-demo accounts (see | ||
| // helpers/auth.ts E2E_FIXTURE_USERS). Under the shared env-driven login the | ||
| // coach/player never saw the seeded camp. | ||
| import { loginAsFixtureCoach as loginAsCoach, loginAsFixturePlayer as loginAsPlayer } from './helpers/auth'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Avoid aliasing fixture imports for clarity and consistency.
Consider using the explicit loginAsFixtureCoach and loginAsFixturePlayer names directly in the test body instead of aliasing them. This improves readability by making it immediately obvious that these tests rely on fixture accounts rather than env-driven accounts, keeping it consistent with baseball-box-score.spec.ts and baseball-pipeline.spec.ts.
♻️ Proposed refactor
-// helpers/auth.ts E2E_FIXTURE_USERS). Under the shared env-driven login the
-// coach/player never saw the seeded camp.
-import { loginAsFixtureCoach as loginAsCoach, loginAsFixturePlayer as loginAsPlayer } from './helpers/auth';
+// helpers/auth.ts E2E_FIXTURE_USERS). Under the shared env-driven login the
+// coach/player never saw the seeded camp.
+import { loginAsFixtureCoach, loginAsFixturePlayer } from './helpers/auth';Note: If applied, ensure you update the downstream await loginAsCoach(page) and await loginAsPlayer(page) calls in this file.
🤖 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/camps.spec.ts` around lines 2 - 7, Remove the aliases from the fixture
helper import in e2e/camps.spec.ts, and update the test body calls from
loginAsCoach and loginAsPlayer to loginAsFixtureCoach and loginAsFixturePlayer
respectively. Keep the fixture-based login behavior unchanged.
| /** Log in as the #375 seed's dedicated fixture COACH (testcoach@helm.test). */ | ||
| export async function loginAsFixtureCoach(page: Page) { | ||
| await page.goto('/baseball/login'); | ||
| await page.fill('input[name="email"]', E2E_FIXTURE_USERS.coach.email); | ||
| await page.fill('input[name="password"]', E2E_FIXTURE_USERS.coach.password); | ||
| await page.click('button[type="submit"]'); | ||
| await page.waitForURL('**/dashboard/**'); | ||
| } | ||
|
|
||
| /** Log in as the #375 seed's dedicated fixture PLAYER (testplayer@helm.test). */ | ||
| export async function loginAsFixturePlayer(page: Page) { | ||
| await page.goto('/baseball/login'); | ||
| await page.fill('input[name="email"]', E2E_FIXTURE_USERS.player.email); | ||
| await page.fill('input[name="password"]', E2E_FIXTURE_USERS.player.password); | ||
| await page.click('button[type="submit"]'); | ||
| // Players land on /baseball/player/*, coaches on /baseball/dashboard/* — use | ||
| // the shared post-login URL contract (mirrors loginAsPlayer above). | ||
| await page.waitForURL(SUCCESS_URL_RE); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Extract shared login steps to a helper function.
loginAsFixtureCoach and loginAsFixturePlayer duplicate the navigation and form-fill logic of loginAsCoach and loginAsPlayer in the same file. Extracting a shared internal helper will reduce boilerplate and standardize the login flow.
♻️ Proposed refactor
+async function performLogin(page: Page, user: { email: string; password: string }, successUrl: string | RegExp) {
+ await page.goto('/baseball/login');
+ await page.fill('input[name="email"]', user.email);
+ await page.fill('input[name="password"]', user.password);
+ await page.click('button[type="submit"]');
+ await page.waitForURL(successUrl);
+}
+
/** Log in as the `#375` seed's dedicated fixture COACH (testcoach@helm.test). */
export async function loginAsFixtureCoach(page: Page) {
- await page.goto('/baseball/login');
- await page.fill('input[name="email"]', E2E_FIXTURE_USERS.coach.email);
- await page.fill('input[name="password"]', E2E_FIXTURE_USERS.coach.password);
- await page.click('button[type="submit"]');
- await page.waitForURL('**/dashboard/**');
+ await performLogin(page, E2E_FIXTURE_USERS.coach, '**/dashboard/**');
}
/** Log in as the `#375` seed's dedicated fixture PLAYER (testplayer@helm.test). */
export async function loginAsFixturePlayer(page: Page) {
- await page.goto('/baseball/login');
- await page.fill('input[name="email"]', E2E_FIXTURE_USERS.player.email);
- await page.fill('input[name="password"]', E2E_FIXTURE_USERS.player.password);
- await page.click('button[type="submit"]');
- // Players land on /baseball/player/*, coaches on /baseball/dashboard/* — use
- // the shared post-login URL contract (mirrors loginAsPlayer above).
- await page.waitForURL(SUCCESS_URL_RE);
+ await performLogin(page, E2E_FIXTURE_USERS.player, SUCCESS_URL_RE);
}📝 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.
| /** Log in as the #375 seed's dedicated fixture COACH (testcoach@helm.test). */ | |
| export async function loginAsFixtureCoach(page: Page) { | |
| await page.goto('/baseball/login'); | |
| await page.fill('input[name="email"]', E2E_FIXTURE_USERS.coach.email); | |
| await page.fill('input[name="password"]', E2E_FIXTURE_USERS.coach.password); | |
| await page.click('button[type="submit"]'); | |
| await page.waitForURL('**/dashboard/**'); | |
| } | |
| /** Log in as the #375 seed's dedicated fixture PLAYER (testplayer@helm.test). */ | |
| export async function loginAsFixturePlayer(page: Page) { | |
| await page.goto('/baseball/login'); | |
| await page.fill('input[name="email"]', E2E_FIXTURE_USERS.player.email); | |
| await page.fill('input[name="password"]', E2E_FIXTURE_USERS.player.password); | |
| await page.click('button[type="submit"]'); | |
| // Players land on /baseball/player/*, coaches on /baseball/dashboard/* — use | |
| // the shared post-login URL contract (mirrors loginAsPlayer above). | |
| await page.waitForURL(SUCCESS_URL_RE); | |
| } | |
| async function performLogin(page: Page, user: { email: string; password: string }, successUrl: string | RegExp) { | |
| await page.goto('/baseball/login'); | |
| await page.fill('input[name="email"]', user.email); | |
| await page.fill('input[name="password"]', user.password); | |
| await page.click('button[type="submit"]'); | |
| await page.waitForURL(successUrl); | |
| } | |
| /** Log in as the `#375` seed's dedicated fixture COACH (testcoach@helm.test). */ | |
| export async function loginAsFixtureCoach(page: Page) { | |
| await performLogin(page, E2E_FIXTURE_USERS.coach, '**/dashboard/**'); | |
| } | |
| /** Log in as the `#375` seed's dedicated fixture PLAYER (testplayer@helm.test). */ | |
| export async function loginAsFixturePlayer(page: Page) { | |
| await performLogin(page, E2E_FIXTURE_USERS.player, SUCCESS_URL_RE); | |
| } |
🤖 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/helpers/auth.ts` around lines 93 - 111, Extract the shared navigation,
credential form filling, and submit steps from loginAsCoach/loginAsPlayer and
the fixture variants into an internal login helper that accepts the page and
user credentials. Update loginAsFixtureCoach and loginAsFixturePlayer to reuse
that helper while preserving their existing post-login URL waits and
role-specific behavior.
Run 29623264998 'passed' these 4 tests only because the chromium step was SKIPPED (the blocking mobile step failed first; no
if:at playwright.yml:213 →success()default). #940 turned the blocking step green, so accessibility.spec + auth.spec executed for the first time and surfaced pre-existing defects:Real product bugs (fixed):
inert(axe color-contrast hasexcludeHidden:false, so aria-hidden alone doesn't exempt it; inert is also the right AT semantics for decoration)Spec drift (fixed in specs, product behavior verified correct):
text=Dashboardnow strict-mode-resolves to 3 elements →getByRole('link',{name:'Dashboard'}).first()?returnTo=…;waitForURL('**/login')glob can't match a query string →/\/login(\?|$)/Flagged for design sign-off (NOT in this PR): white-on-primary-600 is systemic (Button primary/success variants, both login submit buttons) — tracked separately.
🤖 Generated with Claude Code
https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg