Skip to content

Conversation

@joshunrau
Copy link
Collaborator

@joshunrau joshunrau commented Nov 21, 2025

Summary by CodeRabbit

  • Chores
    • Enhanced test environment configuration and isolation.
    • Improved build system port validation and management.
    • Updated end-to-end testing infrastructure for better API endpoint handling.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 21, 2025

Walkthrough

These changes establish a dedicated test environment mode in Vite, add port validation constants, gate the dev authentication bypass to exclude test mode, adjust axios baseURL configuration, and update e2e test setup expectations and webServer URL targeting.

Changes

Cohort / File(s) Change Summary
Build & Environment Configuration
apps/web/package.json, apps/web/vite.config.ts
Updated dev:test script to invoke Vite with --mode test. Added parseNumber import and runtime NaN validation for API_DEV_SERVER_PORT and WEB_DEV_SERVER_PORT; replaced direct parseInt calls with validated port constants.
Authentication Logic
apps/web/src/routes/auth/login.tsx
Added conditional guard to dev bypass flow: authentication bypass now only executes when DEV mode is active AND environment MODE is not 'test'.
HTTP Client Configuration
apps/web/src/services/axios.ts
Changed axios.defaults.baseURL assignment from conditional (test mode check) to unconditional use of config.setup.apiBaseUrl.
E2E Test Configuration
testing/e2e/playwright.config.ts
Updated webServer URL for API dev server from http://localhost:${apiPort} to http://localhost:${apiPort}/v1/setup.
E2E Test Expectations & Page Objects
testing/e2e/src/global/global.setup.spec.ts, testing/e2e/src/pages/__root.page.ts
Changed setup form test expectation from navigation to /dashboard to /auth/login. Made RootPage.$ref property public (removed protected modifier).

Sequence Diagram

sequenceDiagram
    actor User
    participant Login Route
    participant Auth Bypass
    participant Test Mode Check
    
    User->>Login Route: beforeLoad
    Login Route->>Test Mode Check: Is DEV && MODE !== 'test'?
    alt DEV mode, not in test
        Test Mode Check->>Auth Bypass: Proceed with bypass
        Auth Bypass-->>User: Authenticate
    else Test mode or not DEV
        Test Mode Check-->>Login Route: Skip bypass
        Login Route-->>User: Normal auth flow
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Port validation logic in vite.config.ts — verify NaN checks and constant usage are applied consistently
  • Auth bypass conditional in login.tsx — confirm test mode detection prevents unintended bypass execution
  • Axios baseURL change — ensure unconditional assignment doesn't break test isolation or request routing

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main issue being fixed: preventing VITE_DEV_BYPASS_AUTH from being used in tests, which is reflected across multiple files (login.tsx, axios.ts, vite.config.ts, playwright.config.ts, and test expectations).
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 937cc32 and 6f5259b.

📒 Files selected for processing (7)
  • apps/web/package.json (1 hunks)
  • apps/web/src/routes/auth/login.tsx (1 hunks)
  • apps/web/src/services/axios.ts (1 hunks)
  • apps/web/vite.config.ts (3 hunks)
  • testing/e2e/playwright.config.ts (1 hunks)
  • testing/e2e/src/global/global.setup.spec.ts (1 hunks)
  • testing/e2e/src/pages/__root.page.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: lint-and-test
🔇 Additional comments (7)
testing/e2e/src/global/global.setup.spec.ts (1)

14-16: Post-setup URL expectation now matches the non-bypassed auth flow

Updating the expectation from the dashboard to /auth/login correctly reflects that tests no longer auto-login via the dev bypass in test mode.

apps/web/package.json (1)

7-16: Running dev:test with --mode test correctly gates the auth bypass

Having dev:test use vite --mode test ensures import.meta.env.MODE is test, so the new guard in /auth/login prevents the dev bypass from triggering during tests while keeping existing dev behavior.

Please confirm that no other code paths still assume MODE === 'development' when running pnpm dev:test, and that your CI uses this script for e2e runs.

testing/e2e/playwright.config.ts (1)

67-79: API dev server readiness URL correctly targets /v1/setup

Pointing the API webServer url at http://localhost:${apiPort}/v1/setup matches the upstream route that the Vite dev proxy exposes as /api/v1/setup, so Playwright now waits on the real API endpoint instead of the proxied path.

testing/e2e/src/pages/__root.page.ts (1)

4-14: Exposing $ref as public readonly is reasonable for test ergonomics

Making $ref publicly readonly gives tests direct access to the underlying page without compromising immutability, and nicely supports usages like setupPage.expect.toHaveURL(...).

apps/web/src/services/axios.ts (1)

1-9: Unconditional axios baseURL keeps API routing consistent in tests

Always setting axios.defaults.baseURL = config.setup.apiBaseUrl removes the special-casing for test mode, so login/setup calls in tests and dev now consistently target the configured API host rather than the browser origin.

Please double-check that config.setup.apiBaseUrl already includes any /api prefix expected by the backend, and that no callers rely on axios falling back to a different base URL in test mode.

apps/web/vite.config.ts (1)

3-21: Centralized port parsing and validation improve safety; confirm parseNumber semantics

Using parseNumber for apiPort/webPort and throwing when they’re NaN is a good hard-fail on misconfigured dev/test environments, and wiring those values into server.port and the proxy target keeps everything consistent.

Two things to confirm:

  1. That parseNumber returns NaN (not undefined/null) on invalid input so the Number.isNaN(...) checks behave as intended.
  2. That your .env or CI envs always define API_DEV_SERVER_PORT and WEB_DEV_SERVER_PORT, since these checks now run for all Vite commands (including build), and will hard-fail if they’re missing.

Also applies to: 57-65

apps/web/src/routes/auth/login.tsx (1)

81-97: New MODE check correctly disables dev bypass in test environments

Adding import.meta.env.MODE !== 'test' to the bypass condition ensures that VITE_DEV_BYPASS_AUTH only affects normal dev sessions, not dev:test runs, fixing the original test failures while keeping the bypass behavior for everyday development.

After this change and the updated dev:test script, please rerun the e2e suite with VITE_DEV_BYPASS_AUTH enabled to confirm:

  • Setup now lands on /auth/login (not /dashboard).
  • Tests still pass when the bypass flag is off, preserving previous behavior.

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 and usage tips.

@joshunrau joshunrau merged commit 8dee792 into DouglasNeuroInformatics:main Nov 21, 2025
2 checks passed
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