test(dashboard): Playwright end-to-end smoke suite in CI#320
Conversation
GitHub Actions
Inactive
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
WalkthroughAdds Playwright dashboard smoke tests with an isolated gateway configuration, generated asset updates, Vitest test scoping, and a GitHub Actions job that builds the dashboard and runs Chromium-based E2E tests. ChangesDashboard E2E coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
|
I would recommend testing using vitest and the react-testing-library instead of e2e mocked tests that run against a real browser with playwright due to the common downsides of e2e tests being more brittle and slower, etc.. but considering you're testing against the real backend I see there might be some benefit to it |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/otari-dashboard.yml (1)
74-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuote the Python version string in YAML.
A quick tip for the YAML configuration: it's generally a good practice to wrap version numbers in quotes, like
"3.14". While3.14specifically won't lose precision, if someone later changes this to a version ending in a zero (like3.10), YAML parses it as the float3.1before GitHub Actions sees it, which can lead to confusing workflow errors. Quoting it preempts that little headache!✨ Proposed tweak
- uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2 with: - python-version: 3.14 + python-version: "3.14"🤖 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 @.github/workflows/otari-dashboard.yml around lines 74 - 77, Quote the Python version value in the workflow’s setup-uv configuration, changing the unquoted value under python-version to a YAML string while preserving the existing version.
🤖 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 `@web/e2e/dashboard.spec.ts`:
- Around line 15-16: Disable retries for the serial test block configured by
test.describe.configure in web/e2e/dashboard.spec.ts, while preserving its
serial execution mode. Set the block-level retry configuration to zero so CI
does not rerun the stateful describe block against mutated shared database
state.
---
Nitpick comments:
In @.github/workflows/otari-dashboard.yml:
- Around line 74-77: Quote the Python version value in the workflow’s setup-uv
configuration, changing the unquoted value under python-version to a YAML string
while preserving the existing version.
🪄 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
Run ID: dd44b87a-e0e4-42d8-9bfd-0c5a027e8124
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.github/workflows/otari-dashboard.ymlsrc/gateway/static/dashboard/assets/index-Cn8B0tcC.jssrc/gateway/static/dashboard/assets/index-LZ1ofGNu.csssrc/gateway/static/dashboard/index.htmlweb/.gitignoreweb/e2e/dashboard.spec.tsweb/e2e/otari.ymlweb/e2e/serve.shweb/package.jsonweb/playwright.config.tsweb/vite.config.ts
| // One shared gateway + DB, so the flows build on each other and must run in order. | ||
| test.describe.configure({ mode: "serial" }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Disable retries for this serial block to prevent deterministic retry failures.
Hey there! Awesome job getting this E2E suite set up. It’s a great addition.
I noticed a subtle trap with Playwright's serial mode: if a test fails and CI triggers a retry (since retries: 1 in CI), Playwright will retry the entire describe block. Because the shared database is only reset when the webServer boots up, the retried block will start with a mutated database (e.g., the provider added in test 2 will already exist). This will cause test 1 to fail confusingly because it expects the empty-state onboarding screen.
To prevent this, it's best to explicitly disable retries for this stateful block.
🔧 Proposed fix
-// One shared gateway + DB, so the flows build on each other and must run in order.
-test.describe.configure({ mode: "serial" });
+// One shared gateway + DB, so the flows build on each other and must run in order.
+// Retries are disabled because the database is only wiped at server startup;
+// a retried block would inherit the mutated DB state and fail deterministically.
+test.describe.configure({ mode: "serial", retries: 0 });📝 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.
| // One shared gateway + DB, so the flows build on each other and must run in order. | |
| test.describe.configure({ mode: "serial" }); | |
| // One shared gateway + DB, so the flows build on each other and must run in order. | |
| // Retries are disabled because the database is only wiped at server startup; | |
| // a retried block would inherit the mutated DB state and fail deterministically. | |
| test.describe.configure({ mode: "serial", retries: 0 }); |
🤖 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 `@web/e2e/dashboard.spec.ts` around lines 15 - 16, Disable retries for the
serial test block configured by test.describe.configure in
web/e2e/dashboard.spec.ts, while preserving its serial execution mode. Set the
block-level retry configuration to zero so CI does not rerun the stateful
describe block against mutated shared database state.
There was a problem hiding this comment.
Pull request overview
Adds a Playwright end-to-end smoke test layer for the standalone admin dashboard by booting a real gateway (fresh SQLite + migrated to head) and driving core multi-page operator flows in Chromium, with a new CI job to run it.
Changes:
- Added Playwright configuration plus an E2E suite that covers login, first-run onboarding, provider creation, page navigation, and alias creation.
- Added an E2E gateway boot script and dedicated config (fresh SQLite DB, fixed master key, pricing gate disabled) for deterministic runs.
- Wired a new
e2ejob into the dashboard GitHub Actions workflow and scoped Vitest collection toweb/srcso it does not pick up Playwright specs.
Reviewed changes
Copilot reviewed 9 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/vite.config.ts | Limits Vitest test collection to src/** to avoid collecting Playwright specs. |
| web/playwright.config.ts | Playwright runner + webServer config for booting the gateway and running Chromium E2E. |
| web/package.json | Adds Playwright scripts and @playwright/test dev dependency. |
| web/package-lock.json | Locks Playwright dependency tree (@playwright/test, playwright, playwright-core). |
| web/e2e/serve.sh | Boots a fresh standalone gateway for E2E (wipes DB, runs migrations, serves). |
| web/e2e/otari.yml | E2E-only gateway config (SQLite DB, fixed master key, pricing disabled). |
| web/e2e/dashboard.spec.ts | Serial E2E smoke tests for key dashboard workflows. |
| web/.gitignore | Ignores Playwright artifacts and the E2E SQLite DB files. |
| src/gateway/static/dashboard/index.html | Updates hashed asset references after rebuilding the dashboard bundle. |
| .github/workflows/otari-dashboard.yml | Adds an e2e workflow job to build the bundle, install Chromium, and run Playwright. |
Files not reviewed (1)
- web/package-lock.json: Generated file
| webServer: { | ||
| command: "bash e2e/serve.sh", | ||
| url: "http://127.0.0.1:8000/health", | ||
| reuseExistingServer: !process.env.CI, |
| contents: read | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 |
khaledosman
left a comment
khaledosman
left a comment
There was a problem hiding this comment.
E2E/CI smoke suite looks solid; config is internally consistent, ephemeral SQLite files are gitignored, and Vitest is correctly scoped to src/. Two inline notes below. Optional nits: the dashboard and e2e jobs each run npm ci + npm run build from scratch (could share via needs: + an uploaded artifact), and the e2e:install script is unused since CI calls npx playwright install --with-deps chromium directly.
Review created by Claude Code.
| "test": "vitest run", | ||
| "test:watch": "vitest" | ||
| "test:watch": "vitest", | ||
| "e2e": "playwright test", |
There was a problem hiding this comment.
CI builds the bundle before npm run e2e, but this script and serve.sh never build. Locally the suite serves the committed (possibly stale) bundle from src/gateway/static/dashboard, so a dev iterating on web/src gets false pass/fail. Suggest "e2e": "npm run build && playwright test" so local matches CI.
| exit 1 | ||
| fi | ||
|
|
||
| e2e: |
There was a problem hiding this comment.
This job boots the real Python gateway, but the workflow only triggers on web/** and src/gateway/static/dashboard/**. A src/gateway backend change that breaks a dashboard-served flow won't run this suite. Either add the relevant backend paths to the trigger or note the scoping is intentional.
khaledosman
left a comment
khaledosman
left a comment
There was a problem hiding this comment.
One more, on package-manager consistency (see inline).
Review created by Claude Code.
|
|
||
| - name: Install web deps | ||
| working-directory: web | ||
| run: npm ci |
There was a problem hiding this comment.
For consistency with the otari.ai platform (which standardizes on pnpm), web/ here should move off npm to pnpm. Beyond consistency, pnpm is faster, more disk-efficient (content-addressed store with hard links), and safer against supply-chain attacks by default: its strict, non-flat node_modules blocks phantom-dependency access and it does not run install lifecycle scripts unless a package is explicitly allow-listed.
This PR follows the existing npm convention correctly, so don't switch just this new job (that would make the repo internally inconsistent). Instead, do it as a dedicated migration: replace package-lock.json with pnpm-lock.yaml, update both the dashboard and e2e jobs (cache: pnpm, pnpm install --frozen-lockfile, pnpm run ...), and update AGENTS.md. Fine as a follow-up PR.
Adds a Playwright project that boots the gateway serving the built bundle (webServer, SQLite, fixed master key) and walks the core multi-page flows in a real browser: first-run onboarding, adding a provider, navigating the management pages, and creating an alias. Wires an `e2e` job into the dashboard CI workflow, and keeps Vitest scoped to src/ so it does not collect the browser specs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0664f79 to
ec2200b
Compare
@khaledosman Thanks! I'll look into it: yes the idea was that having at least a small subset of playwright driven tests should give highest confidence that things work, but the flakiness and slowness is a definite downside |
…ck db) - `npm run e2e` builds the bundle first so local runs match CI (khaledosman). - webServer reuse is opt-in via PLAYWRIGHT_REUSE_SERVER; default always starts a fresh gateway so a stray :8000 server can't leave dirty DB state (Copilot). - retries: 0 for the serial shared-DB suite (CodeRabbit). - Quote python-version "3.14"; drop the now-redundant CI build step. - Untrack web/e2e/e2e.db* (committed by mistake; already gitignored). - Document the e2e job's web-scoped trigger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Description
Adds Playwright end-to-end coverage for the dashboard, the missing layer above the Vitest component tests.
A Playwright project boots a real gateway (
web/playwright.config.tswebServer→web/e2e/serve.sh: fresh SQLite, migrated to head, a fixed master key, serving the built bundle in standalone mode) and drives a browser through the core multi-page flows:An
e2ejob is wired into the dashboard CI workflow (.github/workflows/otari-dashboard.yml): it sets up Python + uv and Node, builds the bundle, installs Chromium, and runs the suite. Vitest is scoped tosrc/so it does not try to collect the browser specs. The committed bundle shifted only because the new dev dependency bumped the minifier; the app is unchanged (54 Vitest tests still pass).Verified locally:
npx playwright test→ 4 passed (browser against the gateway-served bundle).PR Type
Relevant issues
Closes #310. Part of #299.
Checklist
tests/unit,tests/integration).make lint,make typecheck; web typecheck, Vitest, and the new Playwright suite).AI Usage
AI Model/Tool used: Claude Code (Opus 4.8, 1M context)
Any additional AI details you'd like to share: The agent built and ran the suite (4 passing browser tests against a live gateway) before opening this. Reasoning and decisions are @njbrake's; the code and prose are the agent's.
🤖 Generated with Claude Code
Summary
Benefits