UI Testing with ActRight + Playwright#1294
Merged
6 commits merged intomainfrom Apr 30, 2026
Merged
Conversation
Scaffolds the ActRight / Playwright harness for the Kiln web UI: - Installs @playwright/test and @playwright/mcp (chromium only) in app/web_ui - playwright.config.ts with testDir tests/e2e, baseURL http://localhost:6534, workers: 1 (shared backend), and a webServer array that starts the FastAPI dev_server on port 6535 with HOME redirected to a gitignored .e2e_home dir for state isolation, plus Vite on 6534 - .github/workflows/playwright.yml (monorepo-aware, installs uv + npm deps) - Two sanity tests (act_sanity for install, act_app_loads for the full stack) - Core fixtures in tests/e2e/fixtures.ts: freshState, registeredUser, seededProject, seededProjectWithTask -- backed by /api/projects, /api/tasks, and /api/settings, with act docstrings for ActRight Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Six new act-managed Playwright tests covering the edit task screen: load, save, read-only schemas, clone button, delete flow, and breadcrumb nav. Setup fixes needed to make browser-originated fetches work: - Pass KILN_FRONTEND_PORT=6534 to the backend so its CORS allow-list matches the Vite dev port (defaulted to 5173). - Add globalSetup that polls both servers with real requests, since the webServer url check is a one-shot probe and a page's own fetches can race a still-warming service. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Playwright end-to-end tests, support fixtures and readiness gating, a Playwright config and npm script, .gitignore updates, and a GitHub Actions workflow that runs Playwright tests and uploads HTML reports. Changes
Sequence DiagramssequenceDiagram
participant CI as CI Job (GitHub Actions)
participant Runner as Runner (ubuntu-latest)
participant Repo as Repository
participant Playwright as Playwright Test
participant Artifact as Artifact Storage
CI->>Runner: checkout repo, set working dir ./app/web_ui
Runner->>Runner: setup Node (lts/*), setup Python 3.13, install uv
Runner->>Runner: uv sync (Python deps), npm ci, install Playwright browsers
Runner->>Playwright: npx playwright test (globalSetup + tests)
Playwright-->>Artifact: upload app/web_ui/playwright-report/ (retain 30d)
sequenceDiagram
actor GlobalSetup
participant Req as RequestContext
participant Backend as Backend API (localhost:6535)
participant Frontend as Frontend Web (localhost:6534)
GlobalSetup->>Req: create context for BACKEND_URL and FRONTEND_URL
GlobalSetup->>Backend: poll /api/projects until consecutive 200 responses
Backend-->>Req: 200 JSON or error
GlobalSetup->>Frontend: poll / until 200 and HTML doctype observed
Frontend-->>Req: 200 HTML or error
Req->>Req: dispose
GlobalSetup-->>GlobalSetup: return when both ready
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces an end-to-end testing suite using Playwright, including configuration, global setup for service readiness, and custom fixtures for state management. The changes add several test suites covering core application flows such as task editing, project seeding, and general app loading. Feedback focuses on improving cross-platform compatibility for directory cleanup in the Playwright configuration and ensuring proper resource cleanup in test fixtures to maintain state isolation.
📊 Coverage ReportOverall Coverage: 91% Diff: origin/main...HEADNo lines with coverage information in this diff.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
.github/workflows/playwright.yml (2)
29-32: Browsers cache would speed up CI noticeably.
npx playwright install --with-deps chromiumre-downloads the browser every run. Consider caching~/.cache/ms-playwrightkeyed on the resolved@playwright/testversion (e.g., frompackage-lock.json) and skipping the download on cache hit (still running--with-depsto pull the apt packages, or splitting into a deps-only step).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/playwright.yml around lines 29 - 32, The Playwright step always re-downloads browsers; modify the workflow to cache the Playwright browser cache (~/.cache/ms-playwright) keyed by the resolved `@playwright/test` version (derive hash from package-lock.json or package.json resolution) and change the "npx playwright install --with-deps chromium" step to skip download when the cache is a hit (keep running --with-deps for apt packages or split into a deps-only step). Locate the existing "npx playwright install --with-deps chromium" and "npx playwright test" steps and add a cache restore/save around them using the computed key so the install is skipped on cache hit.
2-6: Consider narrowing triggers to avoid double runs.With both
pushandpull_requestonmain/master, PRs from branches in this repo will run twice (once for the push, once for the PR). A common pattern is to triggerpushonly onmain/master(for post-merge runs) andpull_requestfor everything else, plus adding aconcurrencygroup to cancel superseded runs on force-pushes to a PR:Suggested change
on: push: branches: [ main, master ] pull_request: concurrency: group: playwright-${{ github.workflow }}-${{ github.ref }} cancel-in-progress: ${{ github.event_name == 'pull_request' }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/playwright.yml around lines 2 - 6, The workflow currently triggers both push and pull_request for branches [ main, master ], causing PRs to run twice; update the on: section so push remains limited to branches: [ main, master ] while pull_request has no branch filter (so it runs for all PRs), and add a concurrency block (using a group like playwright-${{ github.workflow }}-${{ github.ref }} and cancel-in-progress: ${{ github.event_name == 'pull_request' }}) to ensure superseded PR runs are cancelled; modify the existing on:, push, pull_request keys and add the new concurrency keys accordingly.app/web_ui/tests/e2e/act_fixtures.spec.ts (1)
84-95: Also assert the fetched task id matches.Symmetric with the
seededProjecttest (line 69) which checksfetched.id === seededProject.id. Without it, the endpoint could return a different task (or stub) and the test would still pass.expect(resp.ok()).toBeTruthy() + const fetched = await resp.json() + expect(fetched.id).toBe(task.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/tests/e2e/act_fixtures.spec.ts` around lines 84 - 95, The test seeded_project_with_task fixture should assert the fetched task ID equals the seeded task ID: after the apiRequest GET call in the test function seededProjectWithTask, parse the response body (e.g. await resp.json()) and add an assertion that the returned task's id matches task.id from the seededProjectWithTask fixture so the endpoint is verified to return the correct resource.app/web_ui/tests/e2e/act_edit_task.spec.ts (1)
220-249: Delete confirmation dialog filter may match too loosely.
page.getByRole("dialog").filter({ hasText: "Delete Task?" })will also match a dialog whose body text happens to contain the substring (e.g., if the title is rendered elsewhere). Consider constraining via an accessible name or heading to make the selector less brittle:- const dialog = page.getByRole("dialog").filter({ hasText: "Delete Task?" }) + const dialog = page.getByRole("dialog", { name: "Delete Task?" })Also, because this test deletes the task,
seededProjectWithTask's teardown will issue a DELETE against the already-removed task. The teardown's.catch(() => {})only swallows thrown errors, not non-2xx responses, so the 404 is silently ignored — this is fine, just noting the interaction is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/tests/e2e/act_edit_task.spec.ts` around lines 220 - 249, The dialog selector is too loose: replace page.getByRole("dialog").filter({ hasText: "Delete Task?" }) with an accessibility-bound selector that targets the dialog by its accessible name or heading (e.g., page.getByRole("dialog", { name: "Delete Task?" }) or locate the dialog then use dialog.getByRole("heading", { name: "Delete Task?" })) so the test matches the actual delete confirmation title reliably; keep the rest of the flow (clicking the Delete button, URL assertion, and API poll) as-is and be aware seededProjectWithTask teardown will attempt to delete the same task but can remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/web_ui/package.json`:
- Line 30: The devDependency "@types/node" in package.json is pinned to
"^25.6.0" which mismatches the CI Node runtime (20.20.2); update the
package.json dependency entry for "@types/node" to a version that matches your
CI Node major (e.g., change "@types/node" to "^20") so type definitions align
with the runtime, or alternatively update the CI Node version to a newer LTS and
pin "@types/node" to that major instead.
In `@app/web_ui/playwright.config.ts`:
- Around line 76-90: The webServer.command currently uses POSIX shell commands
(rm -rf and mkdir -p) which fail on Windows; move the TEST_HOME cleanup out of
the webServer.command into a Node-based setup (e.g., globalSetup) that uses
fs.rmSync(TEST_HOME, { recursive: true, force: true }) and
fs.mkdirSync(TEST_HOME, { recursive: true }) and then set webServer.command to
just start the server ("uv run python -m app.desktop.dev_server"); update the
Playwright config to call that globalSetup and remove the shell cleanup from the
webServer entry so TEST_HOME is created cross-platform and not wiped on
Playwright retries.
- Around line 28-29: The CI retry policy (retries) is causing flaky masking
because tests reuse the same backend process due to fullyParallel: false,
workers: 1 and reuseExistingServer: false; either set retries to 0 (change the
retries variable) until fixtures are isolated, or implement a cleanup/restart
path that wipes the shared fixture directory (e.g., remove or recreate
".e2e_home") and fully restarts the backend between retry attempts so tests like
those depending on freshState see a clean environment; update the playright
config logic around retries and add the cleanup/restart hook invoked before any
retry.
In `@app/web_ui/tests/e2e/act_fixtures.spec.ts`:
- Around line 14-21: The test "fresh_state fixture: backend starts with no
projects" currently only checks resp.ok(); update it to verify the projects list
is empty by parsing the response JSON from apiRequest.get("/api/projects") and
asserting the returned array length is 0 (and optionally that resp.status() ===
200). Use the existing freshState fixture and replace the loose resp.ok()
assertion with explicit checks on resp.json() (or resp.text() parsed to JSON) to
ensure no projects are present.
In `@app/web_ui/tests/e2e/fixtures.ts`:
- Around line 73-95: The seededProject fixture lacks proper teardown so projects
persist and outlive seededProjectWithTask; update the seededProject fixture
(named seededProject) to perform cleanup after await use() by deleting the
created project via the same apiRequest (e.g., call DELETE
/api/projects/${encodeURIComponent(project.id)}) and catch/swallow errors so
Playwright can dispose fixtures in reverse order and seededProjectWithTask’s
task cleanup runs before the project is removed.
- Around line 36-71: The seededProject fixture leaks created projects because it
posts a project in seededProject without deleting it on teardown, causing
freshState (which expects zero projects) to fail for later tests; update
seededProject to perform a best-effort cleanup in its teardown by calling the
project delete endpoint (e.g., DELETE /api/projects/:id using the returned
project.id or slug) after use(), mirroring the cleanup pattern in
seededProjectWithTask; if no delete API exists, instead modify freshState to
actively reset server state or scope freshState to a dedicated worker so tests
do not share persistent HOME=.e2e_home state.
---
Nitpick comments:
In @.github/workflows/playwright.yml:
- Around line 29-32: The Playwright step always re-downloads browsers; modify
the workflow to cache the Playwright browser cache (~/.cache/ms-playwright)
keyed by the resolved `@playwright/test` version (derive hash from
package-lock.json or package.json resolution) and change the "npx playwright
install --with-deps chromium" step to skip download when the cache is a hit
(keep running --with-deps for apt packages or split into a deps-only step).
Locate the existing "npx playwright install --with-deps chromium" and "npx
playwright test" steps and add a cache restore/save around them using the
computed key so the install is skipped on cache hit.
- Around line 2-6: The workflow currently triggers both push and pull_request
for branches [ main, master ], causing PRs to run twice; update the on: section
so push remains limited to branches: [ main, master ] while pull_request has no
branch filter (so it runs for all PRs), and add a concurrency block (using a
group like playwright-${{ github.workflow }}-${{ github.ref }} and
cancel-in-progress: ${{ github.event_name == 'pull_request' }}) to ensure
superseded PR runs are cancelled; modify the existing on:, push, pull_request
keys and add the new concurrency keys accordingly.
In `@app/web_ui/tests/e2e/act_edit_task.spec.ts`:
- Around line 220-249: The dialog selector is too loose: replace
page.getByRole("dialog").filter({ hasText: "Delete Task?" }) with an
accessibility-bound selector that targets the dialog by its accessible name or
heading (e.g., page.getByRole("dialog", { name: "Delete Task?" }) or locate the
dialog then use dialog.getByRole("heading", { name: "Delete Task?" })) so the
test matches the actual delete confirmation title reliably; keep the rest of the
flow (clicking the Delete button, URL assertion, and API poll) as-is and be
aware seededProjectWithTask teardown will attempt to delete the same task but
can remain unchanged.
In `@app/web_ui/tests/e2e/act_fixtures.spec.ts`:
- Around line 84-95: The test seeded_project_with_task fixture should assert the
fetched task ID equals the seeded task ID: after the apiRequest GET call in the
test function seededProjectWithTask, parse the response body (e.g. await
resp.json()) and add an assertion that the returned task's id matches task.id
from the seededProjectWithTask fixture so the endpoint is verified to return the
correct resource.
🪄 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: 4b030e73-dcf2-4f58-baff-ce2115963578
⛔ Files ignored due to path filters (1)
app/web_ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.github/workflows/playwright.ymlapp/web_ui/.gitignoreapp/web_ui/package.jsonapp/web_ui/playwright.config.tsapp/web_ui/tests/e2e/act_app_loads.spec.tsapp/web_ui/tests/e2e/act_edit_task.spec.tsapp/web_ui/tests/e2e/act_fixtures.spec.tsapp/web_ui/tests/e2e/act_sanity.spec.tsapp/web_ui/tests/e2e/fixtures.tsapp/web_ui/tests/e2e/global-setup.ts
Prevents leaked projects from failing the freshState assertion when tests run serially against a shared backend.
Make fixtures really put the app into clean states, and validate each fixture's UI goal in a browser so fixture tests exercise the same paths (CORS, Vite readiness, ui_state redirect) that real UI tests hit. - Centralize frontend/backend ports + URLs in tests/e2e/ports.ts. - Rename freshState -> cleanBackend; actively delete projects and clear settings instead of only asserting empty state. - Have registeredUser and seededProject depend on cleanBackend so every test starts from a reset. - Roll ui_state localStorage priming into seededProjectWithTask; drop the primeUiState helper from act_edit_task.spec.ts. - Rewrite act_fixtures.spec.ts to open a browser and assert the redirect/heading each fixture is supposed to produce. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/web_ui/tests/e2e/act_edit_task.spec.ts (1)
205-205: Delete button locator is brittle to duplicate icons.
button:has(img[src$="/images/delete.svg"])will silently match the first candidate if the page ever renders another button with the same icon (e.g., a row-level delete). A stablearia-label/title/data-testidwould make the intent explicit and survive future markup changes. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/tests/e2e/act_edit_task.spec.ts` at line 205, The current locator "button:has(img[src$=\"/images/delete.svg\"])" is brittle and can match the wrong delete icon; update the test to target a stable attribute (e.g., aria-label, title, or data-testid) on the intended delete control instead of relying on the image selector, and if that attribute doesn't exist add one to the corresponding element in the UI so the test can use something like page.locator('[data-testid="..."]') or locator('button[aria-label="..."]') to unambiguously select the correct delete button.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/web_ui/tests/e2e/act_edit_task.spec.ts`:
- Line 205: The current locator "button:has(img[src$=\"/images/delete.svg\"])"
is brittle and can match the wrong delete icon; update the test to target a
stable attribute (e.g., aria-label, title, or data-testid) on the intended
delete control instead of relying on the image selector, and if that attribute
doesn't exist add one to the corresponding element in the UI so the test can use
something like page.locator('[data-testid="..."]') or
locator('button[aria-label="..."]') to unambiguously select the correct delete
button.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8d8f714-dd16-4398-9173-0c6335f1603a
📒 Files selected for processing (6)
app/web_ui/playwright.config.tsapp/web_ui/tests/e2e/act_edit_task.spec.tsapp/web_ui/tests/e2e/act_fixtures.spec.tsapp/web_ui/tests/e2e/fixtures.tsapp/web_ui/tests/e2e/global-setup.tsapp/web_ui/tests/e2e/ports.ts
✅ Files skipped from review due to trivial changes (1)
- app/web_ui/tests/e2e/ports.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/web_ui/tests/e2e/act_fixtures.spec.ts
- Drop unused @playwright/mcp devDep (was pulling playwright 1.60.0-alpha transitively; .mcp.json uses npx @latest, so no manifest pin is needed) - Pin @types/node to ^22.0.0 to match CI's Node LTS runtime - Playwright workflow: add paths filter, cancel-in-progress concurrency, drop master branch, align setup-uv to @V3 with enable-cache (repo convention) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New UI testing setup.
Should install ActRight skill: https://github.com/scosman/ActRight
1s demo video:
https://github.com/user-attachments/assets/544d2b1e-b8e2-45bf-b01d-0284a676e5d4
Summary by CodeRabbit
Tests
Chores