Skip to content

fix(a11y+e2e): landing/signup contrast (real) + auth spec drift — latent failures unmasked by #940 - #941

Merged
njrini99-code merged 2 commits into
mainfrom
fix/chromium-e2e-triage
Jul 18, 2026
Merged

fix(a11y+e2e): landing/signup contrast (real) + auth spec drift — latent failures unmasked by #940#941
njrini99-code merged 2 commits into
mainfrom
fix/chromium-e2e-triage

Conversation

@njrini99-code

Copy link
Copy Markdown
Owner

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):

  • Landing "Request Demo" + signup gate "Continue": white on primary-600 = 3.29:1 (needs 4.5:1) → primary-700 (~5.0:1), hover primary-800
  • Landing decorative fake-browser mockup URL text 2.63:1 → subtree marked inert (axe color-contrast has excludeHidden: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):

  • auth.spec.ts:12 — text=Dashboard now strict-mode-resolves to 3 elements → getByRole('link',{name:'Dashboard'}).first()
  • auth.spec.ts:152 — redirect correctly carries ?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

…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-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@cursor

cursor Bot commented Jul 18, 2026

Copy link
Copy Markdown

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.

@supabase

supabase Bot commented Jul 18, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project qmnssrrolpinvwjjnufo because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@vercel

vercel Bot commented Jul 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
helmv3 Ignored Ignored Preview Jul 18, 2026 6:14am

Request Review

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review Change Stack

Summary by CodeRabbit

  • Style
    • Refined primary CTA button colors and interaction states on the sign-up and landing experiences.
    • Updated the decorative browser-frame preview to be non-interactive for accessibility.
  • Tests
    • Improved auth E2E assertions for dashboard link targeting and logout-to-login redirects that may include a return parameter.
    • Updated Baseball E2E flows to authenticate with fixture-pinned accounts and made box-score/game discovery more deterministic.
    • Adjusted an anonymous route expectation to confirm redirect to the auth wall.

Walkthrough

Baseball 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.

Changes

Seeded E2E reliability

Layer / File(s) Summary
Add fixture authentication helpers
e2e/helpers/auth.ts:68-112
Adds pinned fixture credentials and coach/player login helpers with role-specific URL waits.
Use fixture accounts in Baseball tests
e2e/baseball-box-score.spec.ts:3-7, 88-96, e2e/baseball-pipeline.spec.ts:2-7, 54, 154, e2e/camps.spec.ts:2-7
Baseball E2E setup uses fixture authentication and retains skip behavior when fixture login is unavailable.
Align Baseball route and game assertions
e2e/baseball-stats-smoke.spec.ts:84-99, 178-186
Anonymous player access expects the auth wall, and completed games are located by manifest game ID.
Align general authentication assertions
e2e/auth.spec.ts:28-37, 166-171
Coach login targets the sidebar Dashboard link, while protected-route checks accept redirects with query parameters.

UI contrast and accessibility

Layer / File(s) Summary
Update primary control contrast
src/app/golf/(auth)/signup/page.tsx:177-180, src/components/landing/Hero.tsx:75-78
Signup and Request Demo buttons use darker primary and hover/active color tokens.
Mark decorative frame inert
src/components/landing/Hero.tsx:262-271
The decorative browser mockup is marked inert and documented as non-interactive accessibility content.

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
Loading

Possibly related issues

  • njrini99-code/helmv3#382: Adds deterministic seeded Baseball E2E authentication and updates Baseball smoke coverage, matching this PR’s fixture-authentication changes.

Possibly related PRs

🚥 Pre-merge checks | ✅ 10 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Title check ⚠️ Warning Uses an unsupported scope (a11y+e2e) under the Conventional Commits requirements, so the title does not satisfy the required scope list. Change the scope to one of the allowed values (for example golf, baseball, or design-system) and keep the title in Conventional Commits form.
Conventional Commits ❓ Inconclusive Squashed commit subject matches the Conventional Commits regex, but the PR title isn't available in repo context to verify. Provide the PR title from GitHub metadata; the current HEAD subject already passes the regex.
✅ Passed checks (10 passed)
Check name Status Explanation
Description check ✅ Passed The description matches the implemented landing, auth-spec, and baseball fixture changes, so it is clearly related to the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No Service-Role In Client Bundles ✅ Passed PASS: No changed code outside admin/api admin dirs adds SUPABASE_SERVICE_ROLE_KEY or a service-role Supabase client; only a preexisting comment in e2e/baseball-stats-smoke.spec.ts:278 mentions it.
Rls Coverage On New Tables ✅ Passed No migration files changed in this PR diff; only e2e/UI files are modified, so the RLS check is not applicable.
Auth Check In Server Actions ✅ Passed No changed files matched src/app//actions//*.ts in this PR diff, so the getUser-before-from/rpc check is not applicable.
Sport-Prefixed Table Names ✅ Passed Only Supabase refs in changed TS/TSX files are baseball_games/baseball_events in e2e/baseball-box-score.spec.ts:10,13,56,82; no unprefixed coach/player/team/round/event tables.
No Destructive Writes ✅ Passed e2e/baseball-box-score.spec.ts:30-34 only deletes rows in teardown; no changed file adds INSERT/UPSERT or delete→insert rebuilds.
No Edits To Historical Migrations ✅ Passed No files under supabase/migrations/ changed in the PR diff, so no historical migration edits occurred.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/chromium-e2e-triage
  • 🛠️ helm safety pass
  • 🛠️ dashboard ux pass
  • 🛠️ rls test pass

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 6da7479 and 159bdc3.

📒 Files selected for processing (3)
  • e2e/auth.spec.ts
  • src/app/golf/(auth)/signup/page.tsx
  • src/components/landing/Hero.tsx

Comment thread e2e/auth.spec.ts
Comment on lines +35 to +37
await expect(page.getByRole('link', { name: 'Dashboard' }).first()).toBeVisible({
timeout: 5000,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 159bdc3 and 28c8c55.

📒 Files selected for processing (5)
  • e2e/baseball-box-score.spec.ts
  • e2e/baseball-pipeline.spec.ts
  • e2e/baseball-stats-smoke.spec.ts
  • e2e/camps.spec.ts
  • e2e/helpers/auth.ts

Comment on lines +96 to +99
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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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.

Comment on lines +184 to +186
const seededGameLink = page.locator(
`a[data-testid="game-card"][href$="/baseball/dashboard/stats/games/${BASEBALL_SEED_MANIFEST.completedGame.id}"]`,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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

Comment thread e2e/camps.spec.ts
Comment on lines +2 to +7
// 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';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Comment thread e2e/helpers/auth.ts
Comment on lines +93 to +111
/** 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);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
/** 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant