test(e2e): run e2e against production build

[kantord]

Summary

• Run E2E tests against production builds instead of development server
• Fix rate limiting issue that caused flaky test failures when multiple tests authenticate in quick succession

Problem

E2E tests were failing intermittently with authentication timeouts. The root cause was Better Auth's default rate limit of 3 sign-in requests
per 10 seconds, enforced based on IP.

Solution

Added a configurable BETTER_AUTH_RATE_LIMIT environment variable that overrides the default rate limit for sign-in paths. For E2E tests, this
is set to 100 to prevent rate limit errors while keeping rate limiting enabled in production.

https://stacklok.slack.com/archives/C072SGY78TS/p1769508282379179

[Copilot]

Pull request overview

This PR updates the E2E testing setup to run against production builds and makes the authentication flow and rate limiting more robust and test-friendly.

Changes:

• Hardened the login E2E test to wait for the sign-in button and for navigation away from /signin, aligning it with the shared login fixture behavior.
• Introduced a configurable Better Auth rate-limit override for sign-in endpoints and wired it into the auth configuration, with environment defaults and E2E-specific values.
• Reworked Playwright config, package scripts, the dev OIDC provider, documentation, and the E2E GitHub Actions workflow so E2E tests run against a production build with a dedicated test model and consistent auth/env behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/e2e/login.spec.ts	Makes the login flow test more robust by waiting for the OIDC/Okta button to appear and for navigation away from /signin before asserting the catalog URL.
src/lib/auth/constants.ts	Adds a BETTER_AUTH_RATE_LIMIT constant derived from the environment to control Better Auth rate limiting, documented specifically for E2E usage.
src/lib/auth/auth.ts	Plumbs BETTER_AUTH_RATE_LIMIT into the Better Auth configuration via rateLimit.customRules for /sign-in/*, relaxing rate limits when the env var is set.
playwright.config.mts	Switches the test web server to pnpm start:e2e (production build), updates OIDC/auth env variables, and sets E2E model-related env vars for Playwright-driven runs.
package.json	Updates test:e2e* scripts to set auth-related env vars, build the app, and then run Playwright; adds a start:e2e helper that starts OIDC, mock API, and the production Next server.
dev-auth/oidc-provider.mjs	Prevents dotenv from overriding existing env vars, expands TTL configuration for tokens and interactions, and removes an unused variable in the interaction middleware.
CLAUDE.md	Updates documentation to state that E2E tests run against a production build, adjusts the E2E commands, and documents the Better Auth rate-limit override used in tests.
.github/workflows/e2e.yml	Adjusts CI E2E env vars to match the new auth/model configuration, adds an explicit production build step before tests, and relies on pnpm test:e2e for running Playwright.

[kantord]

@claude can you please deep review this branch?

[claude]

Claude finished @kantord's task in 2m 10s —— View job

---

Deep Review of PR #283 ✅

Summary

This PR successfully addresses E2E test flakiness by running tests against production builds and making Better Auth rate limiting configurable. The implementation is well-thought-out, follows project conventions, and includes proper documentation updates.

---

🎯 Architectural Assessment

✅ Strengths

1. Root Cause Analysis: Correctly identified Better Auth's 3 requests/10 seconds rate limit as the cause of flaky tests
2. Production Build Testing: Shifting E2E tests to run against production builds is the right approach - it better represents real-world behavior
3. Configurable Rate Limiting: The BETTER_AUTH_RATE_LIMIT env var provides flexibility without hardcoding test-specific values in the auth config
4. Consistent Environment Variables: All three places (CI workflow, package.json scripts, playwright.config.mts) consistently set the same auth-related env vars
5. Documentation Updates: CLAUDE.md properly documents the rate limiting behavior for future maintainers

🎨 Code Quality

TypeScript Strict Mode: ✅ No any types detected
Async/Await: ✅ Proper usage throughout
Type Safety: ✅ Proper parsing with Number.parseInt() and type guards
Comments: ✅ Excellent inline documentation explaining the "why"

---

🔍 Detailed Review by File

.github/workflows/e2e.yml ⚠️

Changes: Added explicit production build step, updated env vars

Issue 1 - Duplicate Builds (🔴 High Priority)

- name: Build production app
  run: pnpm build
  env:
    BETTER_AUTH_SECRET: test-only-not-a-real-better-auth-secret
    OIDC_PROVIDER_ID: oidc

- name: Run Playwright tests
  run: pnpm test:e2e  # This runs `pnpm build` again with different env vars!

The workflow builds the app twice with different environment variables:

1. First build: OIDC_PROVIDER_ID=oidc
2. Second build (in test:e2e script): OIDC_PROVIDER_ID=okta

This wastes CI time and creates confusion about which build is actually tested.

Recommendations:

• Option A (Preferred): Remove the explicit build step and let test:e2e handle it
• Option B: Remove pnpm build from the test:e2e script and ensure the workflow build uses the same env vars as runtime

Fix duplicate builds →

Issue 2 - Environment Variable Inconsistency
The workflow sets OIDC_PROVIDER_ID: oidc in the job env, but test:e2e overrides it to okta. This makes the job-level env var pointless.

---

tests/e2e/login.spec.ts ⚠️

Changes: Added explicit waits for sign-in button and navigation

Issue - Code Duplication (🟡 Medium Priority)

Lines 6-12 duplicate the exact same login flow already in fixtures.ts:

// login.spec.ts lines 6-12
const signInButton = page.getByRole("button", { name: /oidc|okta/i });
await expect(signInButton).toBeVisible({ timeout: 5000 });
await signInButton.click();
await page.waitForURL((url) => !url.pathname.startsWith("/signin"), {
  timeout: 30000,
});

// fixtures.ts lines 8-14 - IDENTICAL
const signInButton = page.getByRole("button", { name: /oidc|okta/i });
await expect(signInButton).toBeVisible({ timeout: 5000 });
await signInButton.click();
await page.waitForURL((url) => !url.pathname.startsWith("/signin"), {
  timeout: 30000,
});

Why this matters: If the login flow changes (e.g., button text, timeout values, wait conditions), you'll need to update it in two places. This violates DRY principles.

Recommendation: Use the authenticatedPage fixture or export the login helper from fixtures.ts to reuse it here.

Fix login duplication →

---

src/lib/auth/auth.ts ✅

Changes: Added conditional rate limit override

Excellent implementation:

• ✅ Properly uses spread operator with conditional logic
• ✅ Clear comments explaining why customRules is needed (special rules take precedence over global max)
• ✅ Only applies when BETTER_AUTH_RATE_LIMIT is set
• ✅ Keeps the 10-second window consistent with Better Auth defaults

Suggestion: Consider logging when rate limit override is active:

...(BETTER_AUTH_RATE_LIMIT && {
  rateLimit: {
    customRules: {
      "/sign-in/*": {
        max: BETTER_AUTH_RATE_LIMIT,
        window: 10,
      },
    },
  },
}),

Add after line 180:

if (BETTER_AUTH_RATE_LIMIT && !IS_PRODUCTION) {
  console.log(`[Auth] Rate limit override active: ${BETTER_AUTH_RATE_LIMIT} requests/10s`);
}

This helps with debugging and makes it obvious in logs when running in test mode.

---

src/lib/auth/constants.ts ✅

Changes: Added BETTER_AUTH_RATE_LIMIT constant

Excellent implementation:

• ✅ Proper type parsing with Number.parseInt()
• ✅ Returns undefined when not set (allows conditional application)
• ✅ Comprehensive comment explaining the use case
• ✅ References the Better Auth source file for traceability

---

dev-auth/oidc-provider.mjs ✅

Changes: Prevent dotenv from overriding env vars, expand TTL configuration

Good improvements:

• ✅ config({ override: false }) prevents Playwright-set env vars from being clobbered
• ✅ Added explicit Interaction, Session, and Grant TTLs for better control
• ✅ Removed unused variable

Minor observation: Line 110-116 adds TTL configuration that wasn't strictly necessary for this PR's goals, but it's a reasonable improvement for completeness.

---

package.json ⚠️

Changes: Updated E2E test scripts to set env vars and build before running

Issue - Hardcoded Values (🟡 Medium Priority)

The E2E scripts hardcode sensitive-looking values:

"test:e2e": "BETTER_AUTH_SECRET=e2e-test-secret-at-least-32-chars-long OIDC_PROVIDER_ID=okta ..."

While these are test-only values, hardcoding them in three places (this file, playwright.config.mts, and .github/workflows/e2e.yml) creates maintenance burden.

Recommendation: Consider creating a .env.e2e file and using dotenv-cli:

"test:e2e": "dotenv -e .env.e2e -- pnpm build && playwright test"

This centralizes E2E configuration and makes it easier to maintain.

---

playwright.config.mts ✅

Changes: Updated webServer command and env vars

Good implementation:

• ✅ Properly uses start:e2e which runs production build
• ✅ Comprehensive env var configuration
• ✅ Good comment explaining rate limit override
• ✅ Conditional webServer based on serverAlreadyRunning check

Observation: Lines 51-54 have an excellent explanatory comment. This is the kind of documentation that makes code maintainable.

---

CLAUDE.md ✅

Changes: Updated E2E testing documentation

Perfect documentation:

• ✅ Clearly states E2E tests run against production builds
• ✅ Updated commands to reflect new behavior
• ✅ Explains the rate limiting behavior and why override is needed
• ✅ Maintains consistency with actual implementation

---

🔒 Security Review

✅ No Security Concerns Identified

1. Rate Limiting Override: Only affects test environments (controlled via env var)
2. Test Credentials: Properly labeled as test-only-not-a-real-*
3. Production Isolation: Rate limit override is optional and won't accidentally leak to production (no default value)
4. No Hardcoded Secrets: All sensitive values are test-only placeholders

---

🧪 Testing & Reliability

✅ Test Reliability Improvements

1. Root Cause Fixed: Rate limiting no longer blocks parallel test execution
2. Production Build Testing: Better matches real-world behavior
3. Explicit Waits: Login test now properly waits for button visibility and navigation
4. CI Passing: All checks passing including E2E tests ✅

Flakiness Risk Assessment

Before this PR: 🔴 High risk of flaky tests (rate limiting)
After this PR: 🟢 Low risk (rate limiting resolved, explicit waits added)

---

📊 Overall Assessment

Category	Rating	Notes
Correctness	🟢 Excellent	Solves the problem effectively
Code Quality	🟢 Excellent	Clean, well-documented, type-safe
Architecture	🟢 Excellent	Configurable, maintainable approach
Testing	🟢 Excellent	Comprehensive, reliable
Documentation	🟢 Excellent	Clear, thorough updates
Security	🟢 Excellent	No concerns identified
Maintainability	🟡 Good	Minor duplication issues

---

🎯 Recommendations Summary

🔴 High Priority (Before Merge)

1. Fix duplicate builds in CI workflow - Currently building twice with different env vars

🟡 Medium Priority (Consider Before Merge)

1. Remove login flow duplication - login.spec.ts duplicates code from fixtures.ts
2. Centralize E2E env vars - Consider using .env.e2e file instead of hardcoding in multiple places

🟦 Low Priority (Nice to Have)

1. Add rate limit override logging - Help developers know when override is active
2. Align env var names - Job-level OIDC_PROVIDER_ID is unused due to script override

---

✅ Approval Recommendation

Status: ✅ Approve with suggestions

This PR successfully solves the E2E flakiness issue and improves test reliability. The implementation is solid, follows project conventions, and includes excellent documentation.

The identified issues are minor and could be addressed in a follow-up PR if desired. The duplicate build issue in CI is the only one I'd consider addressing before merge, as it wastes CI time and could cause confusion.

Great work on the root cause analysis and implementation! 🎉

---

| Branch: run-e2e-against-prod-build