Add Playwright E2E test suite — 66 tests across all pages#303
Merged
nitrobass24 merged 11 commits intodevelopfrom Mar 19, 2026
Merged
Add Playwright E2E test suite — 66 tests across all pages#303nitrobass24 merged 11 commits intodevelopfrom
nitrobass24 merged 11 commits intodevelopfrom
Conversation
Replace the broken Protractor E2E suite with a modern Playwright-based test suite covering all 5 application pages with 66 tests. Structure: - src/e2e-playwright/ — Playwright project with Page Object pattern - 8 spec files: navigation, dashboard, settings, path-pairs, autoqueue, logs, about, theme - 7 page objects: sidebar, dashboard, settings, path-pairs, autoqueue, logs, about - Shared fixtures with API helpers for config/state management Test approach: - Tests run against a live Docker container (no mocking) - Settings tests verify persistence via API round-trip - Path pairs tests verify CRUD with API cleanup - AutoQueue tests set config flags via API before testing - Dashboard tests gracefully skip when no files present - Delete actions test the two-click confirm safety pattern Removed: - src/e2e/ — old Protractor suite (non-functional since 2022) Added Makefile targets: - make test-e2e — headless Playwright tests - make test-e2e-headed — headed mode for debugging - make test-e2e-report — show HTML report Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces legacy Protractor E2E infra with a new Playwright suite under Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as Test Runner (Make / CI)
participant Container as App Container
participant Browser as Playwright Browser
participant API as Backend API
Runner->>Container: start app container (test-e2e-docker)
Runner->>Browser: launch Playwright tests (BASE_URL)
Browser->>Container: GET / (load UI)
Browser->>API: apiGet / apiSetConfig requests (configure & poll)
API-->>Browser: JSON responses
Browser-->>Runner: test results, traces, screenshots
Runner->>Runner: show-report / cleanup container
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
src/docker/test/e2e/ was part of the old Protractor suite — not referenced by CI, Makefile, or any other code. Contains broken compose files referencing undefined services and ancient Chrome Selenium images. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 44-54: The Playwright E2E targets (test-e2e, test-e2e-headed,
test-e2e-report) assume node_modules exists; add a dedicated install target
(e.g., install-node-deps or install) that runs a deterministic install (npm ci
or npm install) and make the three targets depend on it, or alternatively ensure
each target runs the install command before invoking npx playwright; update
test-e2e, test-e2e-headed, and test-e2e-report to call or depend on that install
step so CI/first-time users won’t fail due to missing dependencies.
In `@src/e2e-playwright/playwright.config.ts`:
- Around line 1-21: Add Firefox and WebKit projects to the Playwright
configuration so tests run across more browsers: update the projects array in
the default export from defineConfig (the projects property) to include entries
for { name: "firefox", use: { browserName: "firefox" } } and { name: "webkit",
use: { browserName: "webkit" } } alongside the existing chromium entry; keep
existing settings (baseURL, trace, screenshot, fullyParallel, etc.) unchanged
and ensure the projects array contains all three browser objects.
In `@src/e2e-playwright/tests/about.spec.ts`:
- Around line 12-18: The test "version number is displayed and matches format
vX.Y.Z" destructures the unused page fixture; remove the unused parameter to
avoid the inconsistency. Edit the test declaration so the async callback does
not accept { page } (e.g., async () => ...) and keep the assertions that use
aboutPage.versionText unchanged.
In `@src/e2e-playwright/tests/autoqueue.spec.ts`:
- Around line 12-13: Tests are changing global AutoQueue settings via
apiSetConfig("autoqueue", "enabled", ...) and apiSetConfig("autoqueue",
"patterns_only", ...); capture the original values at the start of each test
(e.g., call apiGetConfig("autoqueue", "enabled") and apiGetConfig("autoqueue",
"patterns_only") or read them once in a beforeEach), then restore those saved
values in an afterEach (or finally) block using apiSetConfig with the original
values so each test leaves the global state unchanged; update all test cases
that call apiSetConfig for these keys (the occurrences using
apiSetConfig("autoqueue", "enabled", ...) and apiSetConfig("autoqueue",
"patterns_only", ...)) to follow this pattern.
- Around line 68-80: The test is asserting backend state immediately after UI
actions (autoqueue.addPattern / autoqueue.getPatternByText /
autoqueue.getRemoveButton) causing races; replace the direct apiGet assertions
with expect.poll() loops that call apiGet("/server/autoqueue/get") and retry
until the patterns array contains (for add/enter flows) or does not contain (for
remove flow) the testPattern; specifically update the add flow around
autoqueue.addPattern(testPattern) to poll for presence, update the enter flow
(the code that types/enters a pattern) similarly, and update the remove flow
after clicking the removeBtn to poll until the API no longer returns
testPattern, using expect.poll(() =>
apiGet(...)).toResolve()/toContain()/not.toContain() as appropriate so tests
wait for backend persistence before asserting.
In `@src/e2e-playwright/tests/dashboard.spec.ts`:
- Around line 110-120: The test "Queue button appears for queueable files"
currently asserts typeof queueVisible which is always "boolean"; update the
assertion to validate presence directly by replacing the final expect with
expect(queueVisible).toBe(true) and ensure the test targets a deterministically
queueable file (or set up a precondition) using
dashboard.getFileRows()/dashboard.getActionButton to locate that file;
alternatively remove this test if it merely duplicates the earlier action-button
visibility test.
In `@src/e2e-playwright/tests/fixtures.ts`:
- Around line 30-39: The selector used inside the waitForFunction in fixtures.ts
may not match the real restart button; update the querySelector in the anonymous
function (the restartBtn lookup used by page.waitForFunction) to target the
actual DOM (e.g., include '.button' or a more specific selector like '.sidebar
.button') or alternatively add a stable data-testid attribute to the Angular
sidebar component so the selector '[data-testid="restart-btn"]' will work
reliably.
In `@src/e2e-playwright/tests/logs.spec.ts`:
- Around line 66-76: Replace brittle fixed delays with Playwright's
auto-retrying expectations: remove page.waitForTimeout(500) calls and instead
wait for the records Locator to reach the expected counts. For the first block
use expect.poll(() => records.count()).toBeLessThanOrEqual(count) (or another
expect.poll assertion that checks records.count()), and for the "no match" case
use await expect(records).toHaveCount(0). Update code around
logs.searchInput.fill(...) / records.count() to use these assertions so the test
waits for a stable condition rather than a fixed timeout.
In `@src/e2e-playwright/tests/navigation.spec.ts`:
- Around line 52-58: The test "active link is highlighted after navigation" uses
an exact href assertion which can break with base paths or hash routing; update
the assertion in that test (the block using sidebar.getActiveLink() and
expect(activeLink).toHaveAttribute(...)) to assert the href contains the
expected segment instead of exact match — e.g., use a partial match (regex or
expect.stringContaining) so the href only needs to include "/settings" rather
than equal it exactly.
In `@src/e2e-playwright/tests/pages/autoqueue.page.ts`:
- Around line 39-42: The addPattern method (using patternInput and addButton)
currently clicks without ensuring the UI has updated; modify addPattern to wait
after click for the new pattern to appear or for relevant network activity to
finish (e.g., wait for the list entry/row that contains the pattern with a
locator.waitFor or wait for the specific XHR/route response) before returning so
subsequent assertions are stable.
- Around line 12-14: The fallback selector for this.patternInput is too broad
(uses "input[type='text']") and can match unrelated inputs; update the locator
on the this.patternInput assignment to scope it to the autoqueue form context
(e.g., use page.locator with a form/container filter or a data-testid/class on
the autoqueue form) so the locator targets the specific input in the autoqueue
component rather than any text input; adjust the selector expression passed to
page.locator (the one that currently contains "input[placeholder*='pattern' i],
input[type='text']") to include the container filter or a more specific
attribute to uniquely identify the pattern field.
In `@src/e2e-playwright/tests/pages/dashboard.page.ts`:
- Around line 22-25: In goto() of dashboard.page.ts, remove the generic
this.page.waitForLoadState("networkidle") and instead use a page-specific
readiness check (e.g., await this.page.waitForSelector(...) for a stable
dashboard element or a data-testid like 'dashboard-ready'), or rely on the
existing waitForStream(page) that the spec runs after navigation; update the
goto() implementation to await that explicit selector or readiness helper so it
doesn't hang on long-lived polling/stream requests.
In `@src/e2e-playwright/tests/pages/logs.page.ts`:
- Around line 29-37: The selectors in getLogRecords and getLogRecordsByLevel are
wrong: they look for classes 'log-record'/'log-line' but the DOM uses class
'record'; update the locator strings in getLogRecords() and
getLogRecordsByLevel(level: string) to target elements with class 'record' and
the level-specific classes (use level.toLowerCase() as before) so both methods
correctly locate records and level-filtered records via this.page.locator with
the corrected CSS classes.
- Around line 13-15: The logHistory locator currently uses
"[class*='log-history'], [class*='log-records']" which doesn't match the
template; update the locator assignment for logHistory to target the actual
class (e.g., ".history-section" or "[class*='history-section']") so the
Playwright page object finds the history container (modify the logHistory
initializer in the logs.page.ts class).
In `@src/e2e-playwright/tests/pages/path-pairs.page.ts`:
- Around line 34-56: The fillForm method currently declares enabled and
autoQueue but never uses them and selects inputs by brittle positional nth()
calls; update fillForm to either remove enabled/autoQueue from the parameter
interface if unused or wire them up to the appropriate toggle controls (e.g.,
locate checkbox/toggle inputs and set their checked state) and replace
positional selectors with resilient, semantic locators for inputs inside the
form (for example use form.locator('input[name="name"]') /
form.locator('input[name="remotePath"]') /
form.locator('input[name="localPath"]') or data-testid/aria-label selectors) so
the function reliably targets the correct fields; keep referencing the fillForm
method and the form locator variable to find and update the named inputs and
toggles.
In `@src/e2e-playwright/tests/pages/settings.page.ts`:
- Around line 49-53: The fixed 500ms sleep after clicking the header (see
collapsed, header and this.page.waitForTimeout) should be replaced with a
condition-based wait: import expect from `@playwright/test` or create a locator
for the section content (e.g., content =
this.page.locator("[class*='card-body']", { hasText: headerText })) and wait for
it to be visible via expect(content).toBeVisible() or content.waitFor({ state:
"visible" }) instead of using waitForTimeout; update the imports to include
expect if you use the expect-based assertion.
In `@src/e2e-playwright/tests/path-pairs.spec.ts`:
- Around line 24-26: The test "empty state shows No path pairs message" assumes
a pristine backend but the suite only cleans after each test; make the initial
state deterministic by clearing/isolating stored path pairs before this test
runs — e.g., add an explicit reset call in a beforeEach or beforeAll hook that
invokes the backend reset helper (create or call a function like
resetPathPairs(), resetDatabase(), or pathPairs.clearStore()) so that when the
test checks pathPairs.emptyMessage it always starts from an empty state;
alternatively, run this test against an isolated test backend or namespace to
guarantee no preexisting path pairs.
- Around line 12-21: The afterEach currently deletes every entry from GET
/server/pathpairs which removes preexisting data; instead capture the pre-test
baseline in test.beforeEach (e.g., call GET /server/pathpairs and store the
initial IDs in a Set on the test context) and then in test.afterEach only delete
pairs whose id is not in that initial set (use the same fetch DELETE flow
referencing pair.id). Update the suite to add test.beforeEach to record
initialPairs and modify test.afterEach to compare against that initialIds set so
only test-created path pairs are removed.
- Around line 45-56: Replace the fixed sleep (page.waitForTimeout(1000)) with a
condition-based wait: poll the path-pairs endpoint or use Playwright's
waitForResponse/locator.waitFor to wait until the created pair appears before
asserting; specifically, replace the waitForTimeout before calling
fetch(`${appUrl}/server/pathpairs`) with a retry loop that refetches the API
until pairs.find((p) => p.name === "e2e-test-pair") is truthy (with a timeout),
or wait for the UI row/locator that represents the new pair to be visible, and
apply the same change for other occurrences of page.waitForTimeout at the noted
lines.
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 102-129: The test "Server Directory field is disabled when path
pairs exist" lacks error handling for the API POST/DELETE used to create and
clean up the path pair; update the setup and teardown fetch calls (the POST to
`${appUrl}/server/pathpairs` and the DELETE to
`${appUrl}/server/pathpairs/${pair.id}`) to check response.ok and throw/assert
with a clear message if the request fails, and guard the DELETE by confirming
pair and pair.id exist (or wrap cleanup in a try/finally) so settings.goto(),
settings.getTextInput("Server Directory"), and the expect toBeDisabled only run
when setup succeeded and cleanup is attempted safely even on failure.
- Around line 29-31: Replace the fragile hardcoded delay
page.waitForTimeout(1500) with an explicit wait for the debounced save to
complete: detect the save completion by waiting for the network request that
performs the save (e.g., using page.waitForResponse or page.waitForRequest
matching the save endpoint/route) or by waiting for a UI indicator to change
(e.g., page.waitForSelector that asserts a "saved" toast or spinner
disappearance). Update the test(s) that currently call page.waitForTimeout(1500)
to use page.waitForResponse/Request or page.waitForSelector so the test reliably
proceeds once the actual save finishes rather than after a fixed timeout.
In `@src/e2e-playwright/tests/theme.spec.ts`:
- Around line 39-44: The two immediate clicks on sidebar.themeToggle can race
with DOM/storage updates; change the test to wait for the HTML data-bs-theme
attribute to update between toggles: capture initialTheme (using
page.locator("html").getAttribute("data-bs-theme")), click sidebar.themeToggle
once, then await the attribute to equal the opposite theme (use
expect(page.locator("html")).toHaveAttribute(...)), then click
sidebar.themeToggle again and await the attribute to equal initialTheme before
asserting restoredTheme; reference sidebar.themeToggle, initialTheme, and
page.locator("html") to locate the changes.
- Around line 27-30: After clicking sidebar.themeToggle there is a race reading
the attribute immediately; replace the immediate getAttribute/assert with a poll
that waits for the attribute to change (e.g., use page.waitForFunction((theme)
=> document.documentElement.getAttribute('data-bs-theme') !== theme,
initialTheme) ), then read the attribute into newTheme and assert it differs
from initialTheme and is one of ["light","dark"]; update the test code around
sidebar.themeToggle, page.locator("html") and newTheme accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2b7b987-5fa7-48d9-b819-3c5ddf3f1ba3
📒 Files selected for processing (36)
Makefilesrc/e2e-playwright/.gitignoresrc/e2e-playwright/package.jsonsrc/e2e-playwright/playwright.config.tssrc/e2e-playwright/tests/about.spec.tssrc/e2e-playwright/tests/autoqueue.spec.tssrc/e2e-playwright/tests/dashboard.spec.tssrc/e2e-playwright/tests/fixtures.tssrc/e2e-playwright/tests/logs.spec.tssrc/e2e-playwright/tests/navigation.spec.tssrc/e2e-playwright/tests/pages/about.page.tssrc/e2e-playwright/tests/pages/autoqueue.page.tssrc/e2e-playwright/tests/pages/dashboard.page.tssrc/e2e-playwright/tests/pages/logs.page.tssrc/e2e-playwright/tests/pages/path-pairs.page.tssrc/e2e-playwright/tests/pages/settings.page.tssrc/e2e-playwright/tests/pages/sidebar.page.tssrc/e2e-playwright/tests/path-pairs.spec.tssrc/e2e-playwright/tests/settings.spec.tssrc/e2e-playwright/tests/theme.spec.tssrc/e2e/.gitignoresrc/e2e/README.mdsrc/e2e/conf.tssrc/e2e/package.jsonsrc/e2e/tests/about.page.spec.tssrc/e2e/tests/about.page.tssrc/e2e/tests/app.spec.tssrc/e2e/tests/app.tssrc/e2e/tests/autoqueue.page.spec.tssrc/e2e/tests/autoqueue.page.tssrc/e2e/tests/dashboard.page.spec.tssrc/e2e/tests/dashboard.page.tssrc/e2e/tests/settings.page.spec.tssrc/e2e/tests/settings.page.tssrc/e2e/tsconfig.jsonsrc/e2e/urls.ts
💤 Files with no reviewable changes (16)
- src/e2e/tests/settings.page.spec.ts
- src/e2e/.gitignore
- src/e2e/README.md
- src/e2e/package.json
- src/e2e/tests/app.spec.ts
- src/e2e/conf.ts
- src/e2e/tsconfig.json
- src/e2e/tests/about.page.ts
- src/e2e/urls.ts
- src/e2e/tests/dashboard.page.spec.ts
- src/e2e/tests/settings.page.ts
- src/e2e/tests/app.ts
- src/e2e/tests/autoqueue.page.ts
- src/e2e/tests/dashboard.page.ts
- src/e2e/tests/autoqueue.page.spec.ts
- src/e2e/tests/about.page.spec.ts
Comment on lines
+44
to
+54
| # Run Playwright E2E tests (headless, requires running container on port 8800) | ||
| test-e2e: | ||
| cd src/e2e-playwright && npx playwright test | ||
|
|
||
| # Run Playwright E2E tests (headed, for debugging) | ||
| test-e2e-headed: | ||
| cd src/e2e-playwright && npx playwright test --headed | ||
|
|
||
| # Show Playwright HTML report | ||
| test-e2e-report: | ||
| cd src/e2e-playwright && npx playwright show-report |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Targets assume npm install has already been run.
The E2E targets directly invoke npx playwright test without ensuring dependencies are installed. First-time users or CI environments will fail if node_modules doesn't exist.
♻️ Option 1: Add an install target and make E2E depend on it
+# Install E2E dependencies (run once or when package.json changes)
+test-e2e-install:
+ cd src/e2e-playwright && npm install && npx playwright install --with-deps chromium
+
# Run Playwright E2E tests (headless, requires running container on port 8800)
-test-e2e:
+test-e2e: test-e2e-install
cd src/e2e-playwright && npx playwright test♻️ Option 2: Add a note in the help text
`@echo` " test-e2e - Run Playwright E2E tests (headless)"
+ `@echo` " (first run: cd src/e2e-playwright && npm install && npx playwright install)"📝 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
| # Run Playwright E2E tests (headless, requires running container on port 8800) | |
| test-e2e: | |
| cd src/e2e-playwright && npx playwright test | |
| # Run Playwright E2E tests (headed, for debugging) | |
| test-e2e-headed: | |
| cd src/e2e-playwright && npx playwright test --headed | |
| # Show Playwright HTML report | |
| test-e2e-report: | |
| cd src/e2e-playwright && npx playwright show-report | |
| # Install E2E dependencies (run once or when package.json changes) | |
| test-e2e-install: | |
| cd src/e2e-playwright && npm install && npx playwright install --with-deps chromium | |
| # Run Playwright E2E tests (headless, requires running container on port 8800) | |
| test-e2e: test-e2e-install | |
| cd src/e2e-playwright && npx playwright test | |
| # Run Playwright E2E tests (headed, for debugging) | |
| test-e2e-headed: | |
| cd src/e2e-playwright && npx playwright test --headed | |
| # Show Playwright HTML report | |
| test-e2e-report: | |
| cd src/e2e-playwright && npx playwright show-report |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 44 - 54, The Playwright E2E targets (test-e2e,
test-e2e-headed, test-e2e-report) assume node_modules exists; add a dedicated
install target (e.g., install-node-deps or install) that runs a deterministic
install (npm ci or npm install) and make the three targets depend on it, or
alternatively ensure each target runs the install command before invoking npx
playwright; update test-e2e, test-e2e-headed, and test-e2e-report to call or
depend on that install step so CI/first-time users won’t fail due to missing
dependencies.
Comment on lines
+1
to
+21
| import { defineConfig } from "@playwright/test"; | ||
|
|
||
| export default defineConfig({ | ||
| testDir: "./tests", | ||
| timeout: 30_000, | ||
| expect: { timeout: 5_000 }, | ||
| fullyParallel: false, // tests share a Docker container per file | ||
| retries: 0, | ||
| reporter: [["html", { open: "never" }], ["list"]], | ||
| use: { | ||
| baseURL: process.env.BASE_URL || "http://localhost:8800", | ||
| trace: "retain-on-failure", | ||
| screenshot: "only-on-failure", | ||
| }, | ||
| projects: [ | ||
| { | ||
| name: "chromium", | ||
| use: { browserName: "chromium" }, | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Well-structured Playwright configuration.
The config correctly disables parallel execution for shared Docker container usage and includes good failure diagnostics (traces and screenshots). Consider adding Firefox and WebKit projects for broader browser coverage when the suite matures:
projects: [
{ name: "chromium", use: { browserName: "chromium" } },
{ name: "firefox", use: { browserName: "firefox" } },
{ name: "webkit", use: { browserName: "webkit" } },
],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e-playwright/playwright.config.ts` around lines 1 - 21, Add Firefox and
WebKit projects to the Playwright configuration so tests run across more
browsers: update the projects array in the default export from defineConfig (the
projects property) to include entries for { name: "firefox", use: { browserName:
"firefox" } } and { name: "webkit", use: { browserName: "webkit" } } alongside
the existing chromium entry; keep existing settings (baseURL, trace, screenshot,
fullyParallel, etc.) unchanged and ensure the projects array contains all three
browser objects.
- Replace all hardcoded delays (waitForTimeout) with condition-based waits using expect.poll() for API state and expect().toHaveAttribute() for DOM state - Fix log page selectors to match actual DOM classes (.record) - Fix dashboard goto() to use domcontentloaded instead of networkidle (SSE keeps connection open indefinitely) - Add save/restore for autoqueue config in beforeEach/afterEach - Add beforeEach cleanup for path-pairs to ensure clean state - Remove unused page destructures - Replace fixed sleep in expandSection with waitFor visible Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace all waitForLoadState('networkidle') with 'domcontentloaded'
across all page objects and specs — SSE stream keeps the connection
open so networkidle never fires
- Add waitForSelector for sidebar nav link after page load to ensure
Angular has rendered before interacting with the DOM
- Fix sidebar page object: restart/theme are <a> elements not <button>
- Fix waitForStream fixture to use sidebar selector instead of
non-existent restart button data-testid
- Update navigateTo to use domcontentloaded
Note: The app's rate limiter (120 req/60s) can be triggered by rapid
test execution. Tests pass individually but may hit 429 when run as
a full suite. Rate limit tuning for test traffic is a follow-up.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The app's rate limiter (120 req/60s) triggers 429 errors when running the Playwright E2E test suite, which makes rapid page loads and API calls. Add an env var to disable rate limiting for test environments. Set SEEDSYNC_DISABLE_RATE_LIMIT=1 when starting the test container: docker run -e SEEDSYNC_DISABLE_RATE_LIMIT=1 ... Also add make test-e2e-docker target that starts a throwaway container with rate limiting disabled, runs tests, and tears it down. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
src/e2e-playwright/tests/navigation.spec.ts (1)
52-58: 🧹 Nitpick | 🔵 TrivialConsider using partial match for href assertion (optional).
The assertion
toHaveAttribute("href", "/settings")uses an exact match. While this works for the current Angular router configuration, using a partial match would be more resilient to potential base URL or routing changes.♻️ Optional improvement
const activeLink = sidebar.getActiveLink(); await expect(activeLink).toBeVisible(); - await expect(activeLink).toHaveAttribute("href", "/settings"); + await expect(activeLink).toHaveAttribute("href", /\/settings/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/navigation.spec.ts` around lines 52 - 58, The test "active link is highlighted after navigation" uses an exact href assertion on the element returned by sidebar.getActiveLink(); change the assertion to a partial match so it tolerates base-path or routing changes — update the expect(activeLink).toHaveAttribute("href", "/settings") check to assert that the href contains "/settings" (e.g. use a substring or regex matcher) while keeping the rest of the test (page.goto, waitForSelector, visibility check) unchanged.src/e2e-playwright/tests/settings.spec.ts (1)
130-157:⚠️ Potential issue | 🟡 MinorAdd error handling for API setup and cleanup.
The test creates a path pair via direct
fetchbut doesn't handle potential API errors. If the POST fails,pair.idwill be undefined, causing the cleanup to fail and potentially leaving test state corrupted.🛡️ Proposed fix with error handling
// Create a path pair via API const res = await fetch(`${appUrl}/server/pathpairs`, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ name: "temp-pair", remote_path: "/remote/test", local_path: "/local/test", enabled: true, }), }); + if (!res.ok) { + throw new Error(`Failed to create path pair: ${res.status}`); + } const pair = await res.json(); // Reload to pick up the new state await settings.goto(); const serverDir = settings.getTextInput("Server Directory"); await expect(serverDir).toBeDisabled(); // Clean up - await fetch(`${appUrl}/server/pathpairs/${pair.id}`, { - method: "DELETE", - }); + const deleteRes = await fetch(`${appUrl}/server/pathpairs/${pair.id}`, { + method: "DELETE", + }); + if (!deleteRes.ok) { + console.warn(`Cleanup failed for path pair ${pair.id}: ${deleteRes.status}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/settings.spec.ts` around lines 130 - 157, Check the POST and DELETE fetch responses when creating/cleaning the path pair in the "Server Directory field is disabled when path pairs exist" test: after the POST to `${appUrl}/server/pathpairs` verify res.ok (or status) and throw or fail the test with a descriptive message if it isn't OK before using pair; only attempt the DELETE cleanup if pair and pair.id are present and check the delete response as well (failing the test or logging if it fails); this involves updating the POST handling around the `res`/`pair` variables and the DELETE handling that uses `pair.id`, leaving settings.goto(), settings.getTextInput("Server Directory"), and the expect assertion 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 `@src/e2e-playwright/tests/autoqueue.spec.ts`:
- Around line 21-34: The loop that checks patterns returned by
apiGet("/server/autoqueue/get") is a no-op because its body only contains a
comment; either remove this dead-code block entirely or implement actual cleanup
by calling the appropriate removal API for each pattern that startsWith("test-")
(e.g., invoke the existing API helper such as an apiDelete or apiPost removal
helper with the pattern variable p) and handle errors per-item so leftover
test-* patterns don't persist; locate the apiGet call and the for (const p of
patterns) loop and replace the comment with the removal API call or delete the
loop if you choose Option 1.
In `@src/e2e-playwright/tests/logs.spec.ts`:
- Around line 66-70: The current assertion polls for records.count() to be <=
count which will often be true immediately and doesn't confirm filtering; change
the check to assert the filtered count is strictly less than the original count
(use logs.searchInput.fill(searchTerm) then poll until records.count() is <
count) or, if the searchTerm may match all records, instead poll until the
visible records stabilize and each visible record contains searchTerm (verify by
inspecting the visible row text). Ensure you reference logs.searchInput,
records.count(), searchTerm and the original count variable when implementing
the fix.
In `@src/e2e-playwright/tests/pages/about.page.ts`:
- Line 10: The locator for the version text (versionText in about.page.ts) uses
a two-segment regex (/v\d+\.\d+/) that is less precise than the test assertion;
update the locator's regex to match the expected three-segment format (vX.Y.Z)
so it aligns with the assertion in about.spec.ts (use a regex matching
v\d+\.\d+\.\d+ for versionText).
In `@src/e2e-playwright/tests/pages/autoqueue.page.ts`:
- Around line 36-38: The selector in getRemoveButton is wrong: it targets a
"button" element and uses the wrong minus character in the regex, so change
patternItem.locator to target the actual element class (div.button) and update
the text regex to include the Unicode minus U+2212 (e.g. match \u2212) alongside
the ASCII hyphen and "remove" (case-insensitive) so the locator matches the
<span>−</span> minus sign and the div.button element.
- Around line 12-15: The selectors in the Page object are incorrect: update the
patternInput locator (used where patternInput is defined) to target
input[type='search'] (and keep the placeholder match if needed) instead of
input[type='text'], and update the addButton locator (used where addButton is
defined) to target the actual element (a div with class "button", e.g.
div.button containing the "+" span) rather than a <button> element so
page.locator targets the real DOM node.
In `@src/e2e-playwright/tests/pages/dashboard.page.ts`:
- Line 14: The selector for the file list locator in dashboard.page.ts uses a
class-based selector but the template uses id="file-list"; update the locator
initialization (this.fileList / page.locator call) to use the id selector
"#file-list" as the primary selector and keep the existing fallback (e.g., ",
[class*='file-list']") so the locator targets the element by id first; ensure
you modify the code where this.fileList is assigned (the page.locator(...)
expression) to reflect that change.
- Around line 28-30: Selector mismatch: update the test locators to match the
Angular template's DOM by replacing usages of "file-row" and ".file-name" with
the actual container/class and nested name structure; specifically, change the
getFileRows() locator (function getFileRows) to target app-file and elements
with class "file" and update any file-name lookups (e.g., functions that read
the file title) to use the nested selector ".name .text .title" so the
Playwright tests select the real DOM nodes.
In `@src/e2e-playwright/tests/pages/settings.page.ts`:
- Around line 44-61: The expandSection(headerText) logic relies on a nonexistent
"collapsed" class; update expandSection (and its header and collapseBody
locators) to determine collapse state by checking the collapse body visibility
instead of a "collapsed" class: locate the header via the existing
"[class*='card-header']" locator, build the collapseBody via the existing
card.locator("[class*='collapse']").filter({ has: this.page.locator("input,
select") }), then if collapseBody.first().isVisible() is false (or its count is
0/hidden), click the header and waitFor collapseBody.first() to become visible;
remove the "[class*='collapsed'], .collapsed" check so the method works with the
template's "collapsible-header" / boolean-controlled rendering.
In `@src/e2e-playwright/tests/path-pairs.spec.ts`:
- Around line 196-224: The test "delete requires two clicks with confirmation"
currently checks confirmation visibility but not that the pair was removed;
after the second click in that test (inside the test block with
pathPairs.goto(), pathPairs.getPairByName("delete-me") and
pathPairs.getDeleteButton(row)), add an assertion that the pair is gone—either
wait for the row locator to be hidden/removed (e.g., assert
row.isHidden()/toBeHidden or expect(row).not.toBeVisible()) or poll the server
API for absence of "delete-me"—so the test verifies the deletion completed.
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 78-95: The "select dropdown changes value and saves to backend"
test changes the Log Format to "json" but does not restore the original value,
which can leak state; update the test that uses settings.getSelect("Log Format")
and select.selectOption("json") to read and store the current value before
changing it, perform the change and verification via
apiGet("/server/config/get"), then restore the original value at the end of the
test (ideally in a finally/cleanup block) by selecting the previously saved
option so test state is isolated.
- Around line 97-103: The test "Advanced LFTP section is collapsed by default"
uses header and collapsed locators but the selector
header.locator("[class*='collapsed'], .collapsed") is incorrect for the
implementation; update the collapsed locator to target the real classes (e.g.,
use "[class*='collapse-indicator']" or select the "collapsible-header" element)
so collapsed = header.locator("[class*='collapse-indicator']") or an equivalent
selector that matches the collapse indicator in the DOM; keep the rest of the
test assertions unchanged.
---
Duplicate comments:
In `@src/e2e-playwright/tests/navigation.spec.ts`:
- Around line 52-58: The test "active link is highlighted after navigation" uses
an exact href assertion on the element returned by sidebar.getActiveLink();
change the assertion to a partial match so it tolerates base-path or routing
changes — update the expect(activeLink).toHaveAttribute("href", "/settings")
check to assert that the href contains "/settings" (e.g. use a substring or
regex matcher) while keeping the rest of the test (page.goto, waitForSelector,
visibility check) unchanged.
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 130-157: Check the POST and DELETE fetch responses when
creating/cleaning the path pair in the "Server Directory field is disabled when
path pairs exist" test: after the POST to `${appUrl}/server/pathpairs` verify
res.ok (or status) and throw or fail the test with a descriptive message if it
isn't OK before using pair; only attempt the DELETE cleanup if pair and pair.id
are present and check the delete response as well (failing the test or logging
if it fails); this involves updating the POST handling around the `res`/`pair`
variables and the DELETE handling that uses `pair.id`, leaving settings.goto(),
settings.getTextInput("Server Directory"), and the expect assertion unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 782aeb11-0f03-41d7-b804-6851affdde54
📒 Files selected for processing (15)
src/e2e-playwright/tests/about.spec.tssrc/e2e-playwright/tests/autoqueue.spec.tssrc/e2e-playwright/tests/fixtures.tssrc/e2e-playwright/tests/logs.spec.tssrc/e2e-playwright/tests/navigation.spec.tssrc/e2e-playwright/tests/pages/about.page.tssrc/e2e-playwright/tests/pages/autoqueue.page.tssrc/e2e-playwright/tests/pages/dashboard.page.tssrc/e2e-playwright/tests/pages/logs.page.tssrc/e2e-playwright/tests/pages/path-pairs.page.tssrc/e2e-playwright/tests/pages/settings.page.tssrc/e2e-playwright/tests/pages/sidebar.page.tssrc/e2e-playwright/tests/path-pairs.spec.tssrc/e2e-playwright/tests/settings.spec.tssrc/e2e-playwright/tests/theme.spec.ts
Comment on lines
+66
to
+70
| // Type a term that exists | ||
| await logs.searchInput.fill(searchTerm); | ||
| await expect | ||
| .poll(async () => await records.count(), { timeout: 5000 }) | ||
| .toBeLessThanOrEqual(count); |
There was a problem hiding this comment.
Assertion will always pass immediately — toBeLessThanOrEqual(count) doesn't verify filtering worked.
The poll checks if the current count is <= count (the original count), which is trivially true before filtering takes effect. This doesn't validate that the search actually narrowed results.
🐛 Proposed fix to verify filtering reduces count
// Type a term that exists
await logs.searchInput.fill(searchTerm);
- await expect
- .poll(async () => await records.count(), { timeout: 5000 })
- .toBeLessThanOrEqual(count);
+ // Wait for the filter to reduce the count (or stay the same if all match)
+ await expect
+ .poll(async () => await records.count(), { timeout: 5000 })
+ .toBeLessThan(count);Alternatively, if the search term might match all records, consider verifying the count stabilizes or that each visible record contains the search term.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e-playwright/tests/logs.spec.ts` around lines 66 - 70, The current
assertion polls for records.count() to be <= count which will often be true
immediately and doesn't confirm filtering; change the check to assert the
filtered count is strictly less than the original count (use
logs.searchInput.fill(searchTerm) then poll until records.count() is < count)
or, if the searchTerm may match all records, instead poll until the visible
records stabilize and each visible record contains searchTerm (verify by
inspecting the visible row text). Ensure you reference logs.searchInput,
records.count(), searchTerm and the original count variable when implementing
the fix.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 91-93: Help output in the Makefile is missing the new
test-e2e-docker target; update the help target (the block that echoes available
targets) to include a line describing test-e2e-docker so users see it when
running make help — add an `@echo` entry similar to the existing test-e2e /
test-e2e-headed / test-e2e-report lines that briefly describes what
test-e2e-docker does.
- Line 4: The .PHONY declaration is missing the Playwright-related target name
test-e2e-docker; update the .PHONY line in the Makefile to include
test-e2e-docker so the Makefile treats that target as phony (preventing
conflicts with same-named files). Locate the .PHONY line where targets like
build, test-e2e, test-e2e-headed, test-e2e-report are listed and append
test-e2e-docker to that list.
- Around line 45-54: The Makefile target test-e2e-docker is swallowing failures:
remove the "|| true" that masks Playwright test failures and make the startup
loop (the curl -sf ... && break) fail hard if the container never becomes
healthy (exit non-zero after the retry loop). Also guarantee container cleanup
regardless of test outcome by capturing the test/loop exit code and always
running docker rm -f seedsync-e2e-test (e.g., use a trap or a short shell block
that saves $? then removes the container and exits with that saved code) so
failures propagate while cleanup is always performed.
In `@src/python/web/web_app_builder.py`:
- Around line 43-47: Add an integration test that exercises the new
SEEDSYNC_DISABLE_RATE_LIMIT branch by invoking WebAppBuilder.build() (or the
existing integration harness that returns the constructed web_app) with the
environment variable SEEDSYNC_DISABLE_RATE_LIMIT set to a truthy value (e.g.,
"1") and assert that install_security_middleware was configured to bypass rate
limiting for the created web_app; specifically, set the env var in the test,
call WebAppBuilder.build(), then send a request or inspect the
web_app/middleware stack to verify rate limiting is disabled (target symbols:
WebAppBuilder.build, install_security_middleware, SEEDSYNC_DISABLE_RATE_LIMIT).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b61d328c-15e6-42d9-912d-9c82c04477ce
📒 Files selected for processing (3)
Makefilesrc/python/web/security.pysrc/python/web/web_app_builder.py
| # Simplified build system for containerized deployment | ||
|
|
||
| .PHONY: all build build-fresh run stop logs clean test test-image size shell help | ||
| .PHONY: all build build-fresh run stop logs clean test test-image test-e2e test-e2e-headed test-e2e-report size shell help |
There was a problem hiding this comment.
Declare test-e2e-docker in .PHONY.
Line [4] adds Playwright targets but misses test-e2e-docker, which can break invocation if a same-named file appears.
♻️ Proposed fix
-.PHONY: all build build-fresh run stop logs clean test test-image test-e2e test-e2e-headed test-e2e-report size shell help
+.PHONY: all build build-fresh run stop logs clean test test-image test-e2e test-e2e-docker test-e2e-headed test-e2e-report size shell help📝 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
| .PHONY: all build build-fresh run stop logs clean test test-image test-e2e test-e2e-headed test-e2e-report size shell help | |
| .PHONY: all build build-fresh run stop logs clean test test-image test-e2e test-e2e-docker test-e2e-headed test-e2e-report size shell help |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 4, The .PHONY declaration is missing the Playwright-related
target name test-e2e-docker; update the .PHONY line in the Makefile to include
test-e2e-docker so the Makefile treats that target as phony (preventing
conflicts with same-named files). Locate the .PHONY line where targets like
build, test-e2e, test-e2e-headed, test-e2e-report are listed and append
test-e2e-docker to that list.
Comment on lines
+45
to
+54
| test-e2e-docker: | ||
| @docker rm -f seedsync-e2e-test 2>/dev/null || true | ||
| docker run -d --name seedsync-e2e-test -p 8801:8800 -e SEEDSYNC_DISABLE_RATE_LIMIT=1 ghcr.io/nitrobass24/seedsync:latest | ||
| @echo "Waiting for container to start..." | ||
| @for i in $$(seq 1 30); do \ | ||
| curl -sf http://localhost:8801/ > /dev/null 2>&1 && break; \ | ||
| sleep 1; \ | ||
| done | ||
| cd src/e2e-playwright && BASE_URL=http://localhost:8801 npx playwright test || true | ||
| @docker rm -f seedsync-e2e-test |
There was a problem hiding this comment.
Do not swallow E2E failures in test-e2e-docker.
Line [53] (|| true) masks test failures, and Line [49]-Line [52] never hard-fails on startup timeout. This can produce false-green CI/local runs.
🛠️ Proposed fix (propagate failures + guaranteed cleanup)
test-e2e-docker:
- `@docker` rm -f seedsync-e2e-test 2>/dev/null || true
- docker run -d --name seedsync-e2e-test -p 8801:8800 -e SEEDSYNC_DISABLE_RATE_LIMIT=1 ghcr.io/nitrobass24/seedsync:latest
- `@echo` "Waiting for container to start..."
- `@for` i in $$(seq 1 30); do \
- curl -sf http://localhost:8801/ > /dev/null 2>&1 && break; \
- sleep 1; \
- done
- cd src/e2e-playwright && BASE_URL=http://localhost:8801 npx playwright test || true
- `@docker` rm -f seedsync-e2e-test
+ `@set` -e; \
+ docker rm -f seedsync-e2e-test 2>/dev/null || true; \
+ trap 'docker rm -f seedsync-e2e-test >/dev/null 2>&1 || true' EXIT; \
+ docker run -d --name seedsync-e2e-test -p 8801:8800 -e SEEDSYNC_DISABLE_RATE_LIMIT=1 ghcr.io/nitrobass24/seedsync:latest; \
+ echo "Waiting for container to start..."; \
+ ready=0; \
+ for i in $$(seq 1 30); do \
+ if curl -sf http://localhost:8801/ >/dev/null 2>&1; then ready=1; break; fi; \
+ sleep 1; \
+ done; \
+ if [ "$$ready" -ne 1 ]; then \
+ echo "Container did not become ready in time"; \
+ exit 1; \
+ fi; \
+ cd src/e2e-playwright && BASE_URL=http://localhost:8801 npx playwright test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 45 - 54, The Makefile target test-e2e-docker is
swallowing failures: remove the "|| true" that masks Playwright test failures
and make the startup loop (the curl -sf ... && break) fail hard if the container
never becomes healthy (exit non-zero after the retry loop). Also guarantee
container cleanup regardless of test outcome by capturing the test/loop exit
code and always running docker rm -f seedsync-e2e-test (e.g., use a trap or a
short shell block that saves $? then removes the container and exits with that
saved code) so failures propagate while cleanup is always performed.
Comment on lines
+91
to
+93
| @echo " test-e2e - Run Playwright E2E tests (headless)" | ||
| @echo " test-e2e-headed - Run E2E tests with browser visible" | ||
| @echo " test-e2e-report - Show Playwright HTML report" |
There was a problem hiding this comment.
Document test-e2e-docker in the help target.
The new target is user-facing but not discoverable via make help.
♻️ Proposed fix
`@echo` " test-e2e - Run Playwright E2E tests (headless)"
+ `@echo` " test-e2e-docker - Run E2E against throwaway Docker container"
`@echo` " test-e2e-headed - Run E2E tests with browser visible"
`@echo` " test-e2e-report - Show Playwright HTML report"📝 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
| @echo " test-e2e - Run Playwright E2E tests (headless)" | |
| @echo " test-e2e-headed - Run E2E tests with browser visible" | |
| @echo " test-e2e-report - Show Playwright HTML report" | |
| `@echo` " test-e2e - Run Playwright E2E tests (headless)" | |
| `@echo` " test-e2e-docker - Run E2E against throwaway Docker container" | |
| `@echo` " test-e2e-headed - Run E2E tests with browser visible" | |
| `@echo` " test-e2e-report - Show Playwright HTML report" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 91 - 93, Help output in the Makefile is missing the
new test-e2e-docker target; update the help target (the block that echoes
available targets) to include a line describing test-e2e-docker so users see it
when running make help — add an `@echo` entry similar to the existing test-e2e /
test-e2e-headed / test-e2e-report lines that briefly describes what
test-e2e-docker does.
Comment on lines
+43
to
+47
| install_security_middleware( | ||
| web_app, | ||
| get_api_key=lambda: self.__context.config.web.api_key, | ||
| disable_rate_limiting=os.environ.get("SEEDSYNC_DISABLE_RATE_LIMIT", "").lower() in ("1", "true", "yes"), | ||
| ) |
There was a problem hiding this comment.
Add integration coverage for the new SEEDSYNC_DISABLE_RATE_LIMIT branch.
Line [46] introduces a new runtime path, but this branch is not exercised by the existing WebAppBuilder.build()-based integration setup. Please add one test that sets the env var and verifies rate-limit bypass behavior.
I can draft a focused integration test in src/python/tests/integration/test_web/test_web_app.py for this path if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/web/web_app_builder.py` around lines 43 - 47, Add an integration
test that exercises the new SEEDSYNC_DISABLE_RATE_LIMIT branch by invoking
WebAppBuilder.build() (or the existing integration harness that returns the
constructed web_app) with the environment variable SEEDSYNC_DISABLE_RATE_LIMIT
set to a truthy value (e.g., "1") and assert that install_security_middleware
was configured to bypass rate limiting for the created web_app; specifically,
set the env var in the test, call WebAppBuilder.build(), then send a request or
inspect the web_app/middleware stack to verify rate limiting is disabled (target
symbols: WebAppBuilder.build, install_security_middleware,
SEEDSYNC_DISABLE_RATE_LIMIT).
Updated selectors based on real DOM inspection: - autoqueue: input[type=search], div.button (not <button>), #controls - dashboard: #file-list, .name .text .title for file names - settings: collapsible-header with *ngIf rendering (not CSS hide) - about: three-segment version regex v\d+.\d+.\d+ Spec fixes: - Removed dead autoqueue cleanup loop - Added row-gone assertion after path-pair delete - Settings: restore log format, check POST/DELETE responses - Advanced LFTP collapse test checks app-option count Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
src/e2e-playwright/tests/path-pairs.spec.ts (1)
8-17:⚠️ Potential issue | 🔴 CriticalDestructive cleanup hooks wipe baseline data.
Line 8 and Line 24 iterate and delete every pair from
/server/pathpairs. This can erase preexisting state and cause cross-test interference when backend state is shared. Scope cleanup to test-created IDs only.Suggested fix
test.describe("Path Pairs", () => { let pathPairs: PathPairsPage; + let baselinePairIds: Set<string>; // Clean up all path pairs before each test to ensure clean state test.beforeEach(async ({ page, appUrl }) => { - const res = await fetch(`${appUrl}/server/pathpairs`); - if (res.ok) { - const pairs = await res.json(); - for (const pair of pairs) { - await fetch(`${appUrl}/server/pathpairs/${pair.id}`, { - method: "DELETE", - }); - } - } + baselinePairIds = new Set(); + const res = await fetch(`${appUrl}/server/pathpairs`); + if (res.ok) { + const pairs: Array<{ id: string }> = await res.json(); + baselinePairIds = new Set(pairs.map((p) => p.id)); + } pathPairs = new PathPairsPage(page); await pathPairs.goto(); }); // Clean up all path pairs after each test test.afterEach(async ({ appUrl }) => { const res = await fetch(`${appUrl}/server/pathpairs`); if (!res.ok) return; - const pairs = await res.json(); + const pairs: Array<{ id: string }> = await res.json(); for (const pair of pairs) { + if (baselinePairIds.has(pair.id)) continue; await fetch(`${appUrl}/server/pathpairs/${pair.id}`, { method: "DELETE", }); } });Also applies to: 24-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/path-pairs.spec.ts` around lines 8 - 17, The beforeEach cleanup currently fetches all entries from the /server/pathpairs endpoint and deletes each one (see test.beforeEach and the loop over pairs -> DELETE to /server/pathpairs/${pair.id}), which wipes baseline data; change this to only delete resources the test created by tracking created IDs: maintain a per-test array (e.g., createdPathPairIds) that you push IDs into when creating pairs, and in test.beforeEach/afterEach only iterate and DELETE those IDs instead of all pairs from the GET; update any helper functions that create path pairs to return or record the created id so the cleanup can target only test-created resources.src/e2e-playwright/tests/settings.spec.ts (1)
147-176:⚠️ Potential issue | 🟡 MinorUse try/finally to ensure cleanup runs on test failure.
If an assertion fails between creating the path pair (line 163) and deleting it (line 172), the test data remains in the system, potentially affecting subsequent test runs. Wrap the test logic in try/finally to guarantee cleanup.
🛡️ Suggested fix
test("Server Directory field is disabled when path pairs exist", async ({ page, appUrl, }) => { // Create a path pair via API const res = await fetch(`${appUrl}/server/pathpairs`, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ name: "temp-pair", remote_path: "/remote/test", local_path: "/local/test", enabled: true, }), }); expect(res.ok).toBe(true); const pair = await res.json(); + try { // Reload to pick up the new state await settings.goto(); const serverDir = settings.getTextInput("Server Directory"); await expect(serverDir).toBeDisabled(); + } finally { + // Clean up regardless of test outcome + await fetch(`${appUrl}/server/pathpairs/${pair.id}`, { + method: "DELETE", + }); + } - - // Clean up - const delRes = await fetch(`${appUrl}/server/pathpairs/${pair.id}`, { - method: "DELETE", - }); - expect(delRes.ok).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/settings.spec.ts` around lines 147 - 176, The test "Server Directory field is disabled when path pairs exist" creates a path pair via fetch and currently deletes it at the end but can leak if assertions fail; wrap the body between creating the pair (the fetch POST returning pair) and the cleanup fetch DELETE into a try/finally block so deletion always runs. Specifically, after obtaining pair from the POST, enter try { await settings.goto(); const serverDir = settings.getTextInput("Server Directory"); await expect(serverDir).toBeDisabled(); } finally { if (pair?.id) { await fetch(`${appUrl}/server/pathpairs/${pair.id}`, { method: "DELETE" }) and assert or ignore the delete result as appropriate } }—use the existing symbols pair, settings.goto, settings.getTextInput, and pair.id to locate and implement the change.src/e2e-playwright/tests/autoqueue.spec.ts (2)
16-21:⚠️ Potential issue | 🟠 MajorTeardown needs to restore the pattern list too.
afterEachonly putsenabledandpatterns_onlyback. These tests also mutate/server/autoqueue/get, and the later UI cleanup clicks are best-effort, so any failure before those lines leaves stale patterns behind in the shared container and future runs. Capture the original pattern set up front and reconcile it in teardown through API helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/autoqueue.spec.ts` around lines 16 - 21, Teardown currently restores only savedEnabled and savedPatternsOnly; capture the original autoqueue pattern list at test start (e.g., call apiGet("/server/autoqueue/get") and store into savedPatterns alongside savedEnabled/savedPatternsOnly) and in test.afterEach restore that savedPatterns via the API helper (use the appropriate API helper—e.g., apiSetConfig or an apiPost to the autoqueue pattern endpoint—to set the pattern list back to savedPatterns). Update the setup/teardown code around test.afterEach, apiGet, apiSetConfig, savedEnabled, and savedPatternsOnly to include savedPatterns so the shared container state is fully reconciled.
214-221:⚠️ Potential issue | 🟠 MajorOrder and tighten the duplicate-path assertion.
The second submit can race the first add, and the current assertion only proves that some error-like element is visible somewhere on the page. Wait until the first row is visible, then assert the exact duplicate message.
🛠️ Suggested fix
const testPattern = `test-dup-${Date.now()}`; await autoqueue.addPattern(testPattern); + await expect(autoqueue.getPatternByText(testPattern)).toBeVisible(); // Try adding the same pattern again await autoqueue.addPattern(testPattern); - const error = autoqueue.getErrorMessage(); - await expect(error).toBeVisible(); + await expect( + autoqueue.getErrorMessage(`Pattern '${testPattern}' already exists.`), + ).toBeVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/autoqueue.spec.ts` around lines 214 - 221, Ensure the test waits for the first added row to appear before asserting the duplicate error: after calling autoqueue.addPattern(testPattern) twice, wait for the row corresponding to testPattern to be visible (e.g., via a helper like autoqueue.waitForPatternRow or a locator that finds the row for testPattern), then fetch the error element with autoqueue.getErrorMessage() and assert its exact text equals the duplicate-path message (replace the current generic toBeVisible assertion with an exact text assertion for the duplicate message).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/e2e-playwright/tests/pages/about.page.ts`:
- Around line 14-18: The goto() method is waiting for an unrelated dashboard
link; change it to wait for an About-page-specific selector instead (e.g., wait
for this.versionText or this.githubLink) to more directly verify the About page
has loaded: in the async goto() function replace or augment the
waitForSelector('a[href="/dashboard"]', ...) call with a wait for the
About-specific element (using the existing this.versionText or this.githubLink
selector) and keep the timeout behavior consistent.
In `@src/e2e-playwright/tests/pages/autoqueue.page.ts`:
- Around line 41-42: The current getErrorMessage() locator is too generic;
change it to assert the concrete duplicate message returned by
autoqueue.service.ts. Update the helper (rename or overload getErrorMessage to
e.g. getDuplicatePatternError(expectedPattern: string)) to wait for a locator
that matches the exact text "Pattern '<pattern>' already exists." (or assert
that the locator's innerText equals that string) so tests call
getDuplicatePatternError(pattern) after onAddPattern() / NotificationService
runs and verify the duplicate-path message rather than any .alert-danger/.error
element. Use the existing getErrorMessage() symbol as the location to modify and
reference onAddPattern() / NotificationService for expected behavior.
In `@src/e2e-playwright/tests/pages/dashboard.page.ts`:
- Around line 50-52: The getActionButton helper currently searches for a
<button> tag but action controls are rendered as <div class="button"> elements;
update getActionButton(fileRow: Locator, action: string) to locate "div.button"
(or locator by role/class) and match the action text case-insensitively
(preserving the existing RegExp(action, "i")) so it finds
Queue/Stop/Extract/Validate/Delete Local/Delete Remote buttons and continues to
respect any disabled attribute/click-guard patterns.
In `@src/e2e-playwright/tests/path-pairs.spec.ts`:
- Around line 92-101: The setup POST calls that create fixtures (the fetch(...)
calls in path-pairs.spec.ts that create "dup-pair" and other path pairs)
currently ignore the response; change each to capture the response (e.g., const
res = await fetch(...)) and immediately assert the request succeeded by calling
expect(res.ok) (or use a shared helper like assertResponseOk(res)) so tests fail
fast on setup failures instead of producing misleading UI assertion errors
later.
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 137-145: Add a short inline comment above the assertion in the
"expanded Advanced LFTP section shows all 7 advanced options" test that explains
the expected count (7) is derived from the OPTIONS_CONTEXT_ADVANCED_LFTP array
in options-list.ts; place the comment near the call to
settings.getSection("Advanced LFTP") or the expect(options).toHaveCount(7) line
and mention OPTIONS_CONTEXT_ADVANCED_LFTP by name so future readers know the
source of the number.
- Around line 78-81: The test callback for test("select dropdown changes value
and saves to backend", ...) destructures an unused page parameter; remove page
from the parameter list (leave apiGet) so the signature becomes async ({ apiGet
}) => { ... } and update any related references to page if present (function
name: the test callback for the "select dropdown changes value and saves to
backend" test) to avoid unused variable lint errors.
- Around line 128-134: The locator for the card body in the test is using
"[class*='collapse']" which matches the header not the body; update the locator
chain in settings.spec.ts (the `body` locator variable) to target the actual
card body element (e.g., use ".card-body" or "app-option") instead of
"[class*='collapse']", then keep the existing `.filter({ has:
page.locator("input, select") })` and assertion `await
expect(body.first()).toBeVisible()` so the test finds the card body that
contains inputs.
- Line 23: The tests "text field change saves to backend" and the checkbox test
(the one destructuring { page, apiGet }) include an unused page parameter;
remove page from the async parameter destructuring in both test functions
(change async ({ page, apiGet }) => to async ({ apiGet }) =>) and verify there
are no remaining references to page in those tests to avoid unused variable
warnings.
---
Duplicate comments:
In `@src/e2e-playwright/tests/autoqueue.spec.ts`:
- Around line 16-21: Teardown currently restores only savedEnabled and
savedPatternsOnly; capture the original autoqueue pattern list at test start
(e.g., call apiGet("/server/autoqueue/get") and store into savedPatterns
alongside savedEnabled/savedPatternsOnly) and in test.afterEach restore that
savedPatterns via the API helper (use the appropriate API helper—e.g.,
apiSetConfig or an apiPost to the autoqueue pattern endpoint—to set the pattern
list back to savedPatterns). Update the setup/teardown code around
test.afterEach, apiGet, apiSetConfig, savedEnabled, and savedPatternsOnly to
include savedPatterns so the shared container state is fully reconciled.
- Around line 214-221: Ensure the test waits for the first added row to appear
before asserting the duplicate error: after calling
autoqueue.addPattern(testPattern) twice, wait for the row corresponding to
testPattern to be visible (e.g., via a helper like autoqueue.waitForPatternRow
or a locator that finds the row for testPattern), then fetch the error element
with autoqueue.getErrorMessage() and assert its exact text equals the
duplicate-path message (replace the current generic toBeVisible assertion with
an exact text assertion for the duplicate message).
In `@src/e2e-playwright/tests/path-pairs.spec.ts`:
- Around line 8-17: The beforeEach cleanup currently fetches all entries from
the /server/pathpairs endpoint and deletes each one (see test.beforeEach and the
loop over pairs -> DELETE to /server/pathpairs/${pair.id}), which wipes baseline
data; change this to only delete resources the test created by tracking created
IDs: maintain a per-test array (e.g., createdPathPairIds) that you push IDs into
when creating pairs, and in test.beforeEach/afterEach only iterate and DELETE
those IDs instead of all pairs from the GET; update any helper functions that
create path pairs to return or record the created id so the cleanup can target
only test-created resources.
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 147-176: The test "Server Directory field is disabled when path
pairs exist" creates a path pair via fetch and currently deletes it at the end
but can leak if assertions fail; wrap the body between creating the pair (the
fetch POST returning pair) and the cleanup fetch DELETE into a try/finally block
so deletion always runs. Specifically, after obtaining pair from the POST, enter
try { await settings.goto(); const serverDir = settings.getTextInput("Server
Directory"); await expect(serverDir).toBeDisabled(); } finally { if (pair?.id) {
await fetch(`${appUrl}/server/pathpairs/${pair.id}`, { method: "DELETE" }) and
assert or ignore the delete result as appropriate } }—use the existing symbols
pair, settings.goto, settings.getTextInput, and pair.id to locate and implement
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f3d6466-1706-4c65-9c9c-81a5b2c9c496
📒 Files selected for processing (7)
src/e2e-playwright/tests/autoqueue.spec.tssrc/e2e-playwright/tests/pages/about.page.tssrc/e2e-playwright/tests/pages/autoqueue.page.tssrc/e2e-playwright/tests/pages/dashboard.page.tssrc/e2e-playwright/tests/pages/settings.page.tssrc/e2e-playwright/tests/path-pairs.spec.tssrc/e2e-playwright/tests/settings.spec.ts
Comment on lines
+14
to
+18
| async goto() { | ||
| await this.page.goto("/about"); | ||
| await this.page.waitForLoadState("domcontentloaded"); | ||
| await this.page.waitForSelector('a[href="/dashboard"]', { timeout: 10_000 }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider waiting for About-specific elements instead of the dashboard link.
The navigation method currently waits for the dashboard link (a[href="/dashboard"]) to ensure the page is ready. While functional, this is an indirect check. Waiting for an About-page-specific element (e.g., this.versionText or this.githubLink) would be more semantically clear and directly verify that the About page content has loaded.
♻️ Suggested refactor
async goto() {
await this.page.goto("/about");
await this.page.waitForLoadState("domcontentloaded");
- await this.page.waitForSelector('a[href="/dashboard"]', { timeout: 10_000 });
+ await this.versionText.waitFor({ timeout: 10_000 });
}📝 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
| async goto() { | |
| await this.page.goto("/about"); | |
| await this.page.waitForLoadState("domcontentloaded"); | |
| await this.page.waitForSelector('a[href="/dashboard"]', { timeout: 10_000 }); | |
| } | |
| async goto() { | |
| await this.page.goto("/about"); | |
| await this.page.waitForLoadState("domcontentloaded"); | |
| await this.versionText.waitFor({ timeout: 10_000 }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e-playwright/tests/pages/about.page.ts` around lines 14 - 18, The
goto() method is waiting for an unrelated dashboard link; change it to wait for
an About-page-specific selector instead (e.g., wait for this.versionText or
this.githubLink) to more directly verify the About page has loaded: in the async
goto() function replace or augment the waitForSelector('a[href="/dashboard"]',
...) call with a wait for the About-specific element (using the existing
this.versionText or this.githubLink selector) and keep the timeout behavior
consistent.
Comment on lines
+50
to
+52
| getActionButton(fileRow: Locator, action: string) { | ||
| return fileRow.locator("button", { hasText: new RegExp(action, "i") }); | ||
| } |
There was a problem hiding this comment.
Selector mismatch: action buttons use <div class="button">, not <button>.
Per the codebase pattern, all action buttons (Queue, Stop, Extract, Validate, Delete Local, Delete Remote) use <div class="button"> elements, not <button> tags. This selector will fail to locate any action buttons.
🔧 Proposed fix
getActionButton(fileRow: Locator, action: string) {
- return fileRow.locator("button", { hasText: new RegExp(action, "i") });
+ return fileRow.locator(".button, [class*='button']", { hasText: new RegExp(action, "i") });
}Based on learnings: "all action buttons (Queue, Stop, Extract, Validate, Delete Local, Delete Remote) intentionally use <div class="button"> elements with [attr.disabled] and click-guard pattern."
📝 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
| getActionButton(fileRow: Locator, action: string) { | |
| return fileRow.locator("button", { hasText: new RegExp(action, "i") }); | |
| } | |
| getActionButton(fileRow: Locator, action: string) { | |
| return fileRow.locator(".button, [class*='button']", { hasText: new RegExp(action, "i") }); | |
| } |
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 50-50: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(action, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e-playwright/tests/pages/dashboard.page.ts` around lines 50 - 52, The
getActionButton helper currently searches for a <button> tag but action controls
are rendered as <div class="button"> elements; update getActionButton(fileRow:
Locator, action: string) to locate "div.button" (or locator by role/class) and
match the action text case-insensitively (preserving the existing RegExp(action,
"i")) so it finds Queue/Stop/Extract/Validate/Delete Local/Delete Remote buttons
and continues to respect any disabled attribute/click-guard patterns.
Comment on lines
+92
to
+101
| await fetch(`${appUrl}/server/pathpairs`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| name: "dup-pair", | ||
| remote_path: "/remote/dup", | ||
| local_path: "/local/dup", | ||
| enabled: true, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Fail fast when setup API calls fail.
These fixture-creation POST calls ignore response status. If one fails, tests proceed with invalid setup and fail later with misleading UI assertions. Add explicit expect(res.ok) (or a shared helper) after each mutation.
Suggested fix
+async function expectOk(res: Response, action: string) {
+ expect(res.ok, `${action} failed: ${res.status}`).toBeTruthy();
+}
// Create a pair via API first
- await fetch(`${appUrl}/server/pathpairs`, {
+ const createRes = await fetch(`${appUrl}/server/pathpairs`, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
name: "dup-pair",
remote_path: "/remote/dup",
local_path: "/local/dup",
enabled: true,
}),
});
+ await expectOk(createRes, "Create dup-pair");Apply the same pattern to each setup POST in this file.
Also applies to: 124-133, 154-163, 201-210, 231-240, 272-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e-playwright/tests/path-pairs.spec.ts` around lines 92 - 101, The setup
POST calls that create fixtures (the fetch(...) calls in path-pairs.spec.ts that
create "dup-pair" and other path pairs) currently ignore the response; change
each to capture the response (e.g., const res = await fetch(...)) and
immediately assert the request succeeded by calling expect(res.ok) (or use a
shared helper like assertResponseOk(res)) so tests fail fast on setup failures
instead of producing misleading UI assertion errors later.
Comment on lines
+78
to
+81
| test("select dropdown changes value and saves to backend", async ({ | ||
| page, | ||
| apiGet, | ||
| }) => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused page parameter.
The page parameter is destructured but not used in this test.
♻️ Suggested fix
- test("select dropdown changes value and saves to backend", async ({
- page,
- apiGet,
- }) => {
+ test("select dropdown changes value and saves to backend", async ({ apiGet }) => {📝 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
| test("select dropdown changes value and saves to backend", async ({ | |
| page, | |
| apiGet, | |
| }) => { | |
| test("select dropdown changes value and saves to backend", async ({ apiGet }) => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e-playwright/tests/settings.spec.ts` around lines 78 - 81, The test
callback for test("select dropdown changes value and saves to backend", ...)
destructures an unused page parameter; remove page from the parameter list
(leave apiGet) so the signature becomes async ({ apiGet }) => { ... } and update
any related references to page if present (function name: the test callback for
the "select dropdown changes value and saves to backend" test) to avoid unused
variable lint errors.
Comment on lines
+137
to
+145
| test("expanded Advanced LFTP section shows all 7 advanced options", async ({ | ||
| page, | ||
| }) => { | ||
| await settings.expandSection("Advanced LFTP"); | ||
|
|
||
| const section = settings.getSection("Advanced LFTP"); | ||
| const options = section.locator("app-option"); | ||
| await expect(options).toHaveCount(7); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a comment explaining the expected count.
The assertion toHaveCount(7) matches the actual OPTIONS_CONTEXT_ADVANCED_LFTP array in options-list.ts. Adding a brief comment noting where this count comes from would aid future maintainability.
📝 Suggested improvement
test("expanded Advanced LFTP section shows all 7 advanced options", async ({
page,
}) => {
await settings.expandSection("Advanced LFTP");
const section = settings.getSection("Advanced LFTP");
const options = section.locator("app-option");
+ // Count matches OPTIONS_CONTEXT_ADVANCED_LFTP.options in options-list.ts
await expect(options).toHaveCount(7);
});📝 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
| test("expanded Advanced LFTP section shows all 7 advanced options", async ({ | |
| page, | |
| }) => { | |
| await settings.expandSection("Advanced LFTP"); | |
| const section = settings.getSection("Advanced LFTP"); | |
| const options = section.locator("app-option"); | |
| await expect(options).toHaveCount(7); | |
| }); | |
| test("expanded Advanced LFTP section shows all 7 advanced options", async ({ | |
| page, | |
| }) => { | |
| await settings.expandSection("Advanced LFTP"); | |
| const section = settings.getSection("Advanced LFTP"); | |
| const options = section.locator("app-option"); | |
| // Count matches OPTIONS_CONTEXT_ADVANCED_LFTP.options in options-list.ts | |
| await expect(options).toHaveCount(7); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e-playwright/tests/settings.spec.ts` around lines 137 - 145, Add a
short inline comment above the assertion in the "expanded Advanced LFTP section
shows all 7 advanced options" test that explains the expected count (7) is
derived from the OPTIONS_CONTEXT_ADVANCED_LFTP array in options-list.ts; place
the comment near the call to settings.getSection("Advanced LFTP") or the
expect(options).toHaveCount(7) line and mention OPTIONS_CONTEXT_ADVANCED_LFTP by
name so future readers know the source of the number.
Comprehensive DOM-verified selector fixes across all page objects and spec files: Page objects: - dashboard: Bootstrap dropdown selectors, #file-list, .actions button - settings: XPath-based section finding, card body visibility check - path-pairs: .path-pairs, .pair-row, .btn-add, .btn-edit, .btn-delete - autoqueue: #add-pattern .button, input[type=search], force click - logs: #log-search, #log-level, .log-filters, #logs .record Specs: - dashboard: dropdown click-to-open pattern for filter/sort tests - settings: correct section names, config path fixes, CSRF headers - path-pairs: wait for SSE delivery, CSRF Origin headers on mutations - autoqueue: afterEach cleanup, enabled state wait, pattern API format - logs: log filters visibility test - Added apiFetch fixture with Origin header for CSRF-protected endpoints 11 tests skip by design (no files on remote, no log records). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Run E2E tests in the build-test job after the Docker image builds: 1. Start container with SEEDSYNC_DISABLE_RATE_LIMIT=1 2. Install Playwright + Chromium 3. Run 66 tests against the live container 4. Upload HTML report artifact on failure 5. Always stop the container This runs on PRs (same trigger as build-test). Release builds use the separate build-amd64/build-arm64 jobs which don't run E2E. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
package-lock.json is gitignored at the repo root level, so npm ci fails with "missing lockfile". Use npm install instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unused page destructures from settings spec tests - Wrap path-pair-dependent test in try/finally so cleanup always runs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (7)
src/e2e-playwright/tests/logs.spec.ts (1)
62-70:⚠️ Potential issue | 🟡 MinorThis poll doesn't actually prove the search filter worked.
records.count() <= countis already true before the filter applies, and on a live SSE-backed list the count can also change independently of the search. Useexpect.pollfor a deterministic post-filter condition instead—e.g. that every visible record containssearchTerm—rather than comparing against the pre-search count. Playwright recommends auto-retrying assertions for asynchronous UI state and providesexpect.pollfor retried custom conditions. (playwright.dev)♻️ Suggested change
// Get text from the first record to use as a valid search term const firstText = await records.first().textContent(); const searchTerm = firstText?.trim().split(/\s+/).pop() || ""; + expect(searchTerm).not.toBe(""); // Type a term that exists await logs.searchInput.fill(searchTerm); await expect - .poll(async () => await records.count(), { timeout: 5000 }) - .toBeLessThanOrEqual(count); + .poll(async () => { + const texts = await records.allTextContents(); + return texts.length > 0 && texts.every((text) => text.includes(searchTerm)); + }, { timeout: 5000 }) + .toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/logs.spec.ts` around lines 62 - 70, The current poll asserting records.count() <= count doesn't verify the filter; change the expect.poll usage after typing into logs.searchInput to poll a deterministic post-filter condition using records and searchTerm: repeatedly fetch visible record texts (via records.nth(i).textContent() or records.allTextContents()) and assert every returned text includes the searchTerm (trimmed/lowercased if needed) so the assertion only passes once the UI has actually filtered to matching items; keep using expect.poll for auto-retry and reference records, logs.searchInput, searchTerm, and expect.poll when locating the change.src/e2e-playwright/tests/path-pairs.spec.ts (2)
91-102:⚠️ Potential issue | 🟠 MajorFail fast on fixture-creation
POSTfailures.These setup requests ignore
res.ok, so a rejected create turns into laterwaitFor()or poll timeouts that point at the wrong assertion. Capture each response and assert success immediately, ideally through a small helper reused across the file.🛠️ One way to make setup failures obvious
test.describe("Path Pairs", () => { let pathPairs: PathPairsPage; + + function expectOk(res: Response, action: string) { + expect(res.ok, `${action} failed: ${res.status}`).toBe(true); + } @@ - await apiFetch("/server/pathpairs", { + const res = await apiFetch("/server/pathpairs", { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ name: "dup-pair", remote_path: "/remote/dup", local_path: "/local/dup", enabled: true, }), }); + expectOk(res, "Create dup-pair");Also applies to: 126-135, 155-164, 203-212, 234-243, 276-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/path-pairs.spec.ts` around lines 91 - 102, The POST setup requests in tests (e.g., inside the "duplicate name shows error message" test using apiFetch) do not check the response, causing silent failures later; update these fixtures to fail fast by capturing the response from apiFetch, assert that res.ok (or res.status is in the expected range), and throw or call test.fail/expect on non-OK responses; factor this into a small helper (e.g., createFixture or postJsonHelper) that sends JSON with headers, checks res.ok, and returns parsed JSON so all setup calls (the POSTs currently using apiFetch across the file) can reuse it and immediately surface creation errors.
7-17:⚠️ Potential issue | 🔴 CriticalDo not delete the entire path-pair collection in setup/teardown.
These hooks remove every existing pair before and after each test. Against a live container that destroys preexisting data and can interfere with any other spec touching path pairs. Capture the baseline IDs and only delete records created by the current test, or run this file against an isolated backend.
Also applies to: 23-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/path-pairs.spec.ts` around lines 7 - 17, The current test.beforeEach unconditionally deletes all path-pair records via apiFetch("/server/pathpairs"), which removes preexisting data; instead capture the baseline set of IDs before tests run (e.g., in test.beforeAll or at the start of test.beforeEach) by fetching "/server/pathpairs" and storing their ids, then in test.afterEach only delete path-pairs created by the test (either by tracking created IDs returned from your test helpers or by deleting any ids from the fetched list that are not in the baseline). Update uses of apiFetch and the deletion loop (the call to apiFetch(`/server/pathpairs/${pair.id}`, { method: "DELETE" })) to consult the baseline set and only issue DELETE for IDs that are test-created/non-baseline; also ensure the same approach is mirrored for the block referenced in test.afterEach.src/e2e-playwright/tests/pages/path-pairs.page.ts (1)
33-52: 🧹 Nitpick | 🔵 Trivial
fillForm()advertises fields it silently ignores.
enabledandautoQueueare part of the method signature, but the implementation only fills the three text inputs. Either wire both checkboxes up here or drop those properties so callers do not assume the page object is setting them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/pages/path-pairs.page.ts` around lines 33 - 52, The fillForm function currently accepts enabled and autoQueue but only fills three text inputs; either remove those properties or wire them to the actual checkbox controls. Update fillForm (function name) to locate the checkboxes inside the .pair-form locator (e.g., form.locator('input[type="checkbox"]') or form.getByRole('checkbox')) and for fields.enabled and fields.autoQueue check whether each is defined, then query the current state (isChecked) and call the appropriate action (check or uncheck) to match the provided boolean; alternatively, if you choose not to support them, remove enabled and autoQueue from the method signature so callers won’t expect the page object to toggle those checkboxes.src/e2e-playwright/tests/pages/dashboard.page.ts (1)
60-63:⚠️ Potential issue | 🟠 Major
getActionButton()is searching the wrong element type.The file-row actions are rendered as
.buttondivs, not<button>elements, so this helper resolves to nothing for queue/stop/delete actions. Switch the locator to the actual control class before the specs rely on it.🔧 Minimal selector fix
getActionButton(fileRow: Locator, action: string) { - return fileRow.locator(".actions button", { + return fileRow.locator(".actions .button", { hasText: new RegExp(action, "i"), }); }Based on learnings: all file action buttons intentionally render as
<div class="button">elements with a click-guard pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/pages/dashboard.page.ts` around lines 60 - 63, getActionButton currently queries for a <button> inside the file row but the UI renders actions as <div class="button">, so update getActionButton (the method using fileRow.locator(".actions button", ...)) to target the actual control (e.g., fileRow.locator(".actions .button", { hasText: new RegExp(action, "i") })) so the helper resolves queue/stop/delete controls; keep the case‑insensitive hasText RegExp and preserve existing signature and return type.src/e2e-playwright/tests/autoqueue.spec.ts (1)
226-245:⚠️ Potential issue | 🟠 MajorWait for the first add to persist before testing duplicates.
The second
addPattern()can race the first write on a live backend, so this occasionally exercises two creates instead of the duplicate-error path. Poll/server/autoqueue/getor wait for the first row to render before the second add.🛠️ Minimal fix
test("duplicate pattern shows error", async ({ page, apiSetConfig, + apiGet, waitForStream, }) => { @@ const testPattern = `test-dup-${Date.now()}`; await autoqueue.addPattern(testPattern); + await expect + .poll( + async () => { + const data = await apiGet("/server/autoqueue/get"); + const raw = Array.isArray(data) ? data : data.patterns || []; + return raw.map((p: any) => (typeof p === "string" ? p : p.pattern)); + }, + { timeout: 5000 } + ) + .toContain(testPattern); // Try adding the same pattern again await autoqueue.addPattern(testPattern);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/autoqueue.spec.ts` around lines 226 - 245, The test races the first addPattern with the second, so after calling AutoQueuePage.addPattern(testPattern) you must wait until the pattern is persisted and rendered before calling addPattern again; update the test to poll the backend endpoint (e.g., /server/autoqueue/get) or use a UI-ready helper such as waitForStream or a new AutoQueuePage.waitForRow(testPattern) to ensure the first create completed, then call AutoQueuePage.addPattern(testPattern) a second time and assert AutoQueuePage.getErrorMessage() is visible.src/e2e-playwright/tests/dashboard.spec.ts (1)
103-110:⚠️ Potential issue | 🟠 MajorFix the row-action checks to match the real dashboard DOM.
row.locator(".actions button")will never see these controls because the file actions render as.buttondivs, and the queue test'sexpect(typeof queueVisible).toBe("boolean")passes whether the queue control exists or not. Use the real selector for the action-strip test, and only assert queue visibility after you establish a known queueable row or drop that second test as redundant.Based on learnings: all file action buttons intentionally render as
<div class="button">elements with a click-guard pattern.Also applies to: 113-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e-playwright/tests/dashboard.spec.ts` around lines 103 - 110, The test "clicking a file row shows action buttons" incorrectly queries row.locator(".actions button") — the dashboard renders action controls as <div class="button"> elements with a click-guard pattern — and the queue check uses a weak typeof assertion; update the selector in that test (and the similar block at 113-123) to target ".actions .button" (or the exact ".button" pattern used by dashboard.getFileRows()/action strip) and replace the loose queue assertion so you only assert queue visibility after selecting a row known to be queueable (or remove the redundant queue check). Locate these changes in the test named "clicking a file row shows action buttons", the use of dashboard.getFileRows(), and the variable/expectation named queueVisible to apply the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 185-206: The Playwright E2E steps currently in the build-test job
are not run for the same contexts that trigger the publish path
(build-amd64/publish), so add the E2E gate to the publish flow by extracting the
Playwright steps ("Set up Node.js", "Install Playwright", "Run E2E tests",
"Upload Playwright report") into a reusable job (e.g., e2e-playwright) or
workflow-call that both build-test and the publish sequence depend on; then make
build-amd64/publish "needs" include that e2e-playwright job (or call it from the
publish job) and preserve the failure() artifact upload behavior so publishing
waits for the E2E suite to pass.
In `@src/e2e-playwright/.gitignore`:
- Around line 1-4: Move the Playwright ignore rules (node_modules/,
playwright-report/, test-results/) out of this nested .gitignore and add them to
the repository root .gitignore (or change the test output location so artifacts
are always emitted under the e2e suite); remove the duplicate test-results/
entry from this file so root-level artifacts like test-results/.last-run.json
are ignored and avoid tracking already-indexed files by updating the root
.gitignore and untracking any currently committed artifacts with git rm --cached
if needed.
In `@src/e2e-playwright/tests/fixtures.ts`:
- Around line 13-14: The helper waitForStream currently only waits for the shell
chrome (the a[href="/dashboard"] anchor) which duplicates Page.goto and does not
guarantee SSE/backend data arrival; update waitForStream (or rename it) to wait
for a page-specific "stream ready" signal instead — e.g. wait for a
dashboard-specific DOM change or test hook that the SSE has populated (such as
the dashboard file count element becoming visible/updated or a dedicated
data-test="stream-ready" marker) or await the actual SSE/endpoint response, so
callers that rely on backend state (like fileCount checks) run against a fully
populated UI; if you cannot add such a signal, rename waitForStream to reflect
that it only waits for chrome (e.g., waitForShellChrome) and update callers
accordingly.
In `@src/e2e-playwright/tests/pages/autoqueue.page.ts`:
- Around line 36-51: The addPattern method currently uses a fixed 200ms wait and
a forced click which can race the button state; update addPattern (referencing
addPattern, patternInput, and addButton) to remove the waitForTimeout and
instead wait until the addButton's disabled attribute is cleared (or its
[attr.disabled] is absent) using a Playwright wait (e.g., waitForFunction or
waitForSelector checking the element is enabled) and then perform a normal click
without { force: true }; this ensures the test synchronizes on the real
clickable state of the div button rather than a sleep.
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 35-50: The persistence tests use fixed targets and can pass
spuriously or leave state mutated; update the "text field change saves to
backend" test (using settings.getTextInput, apiGet and expect.poll) to first
read the current remote_address via apiGet("/server/config/get"), choose a
different testValue when the current equals "e2e-test-server", then perform the
field.clear()/field.fill() and poll as before, and finally restore the original
value in a finally block (using api POST/PUT same endpoint or the UI flow you
already use) to ensure cleanup; apply the same pattern to the other Server
Address test blocks (lines referenced around 90-122 and 189-197) and change the
log_format test to pick a non-current target before toggling while keeping its
restore step.
- Around line 158-186: In the "Server Directory field is disabled when path
pairs exist" test, ensure the temporary path-pair is always deleted by moving
the DELETE call into a finally block: declare a variable to hold the created
pair/id before the POST (e.g., let pairId;), assign pair.id after the successful
apiFetch POST, and perform the apiFetch DELETE using that pairId inside a
finally block so cleanup runs regardless of assertion failures; keep using
settings.getTextInput("Server Directory") and the same apiFetch calls but
guarantee deletion even if res.ok or the expect(serverDir).toBeDisabled()
assertion throws.
---
Duplicate comments:
In `@src/e2e-playwright/tests/autoqueue.spec.ts`:
- Around line 226-245: The test races the first addPattern with the second, so
after calling AutoQueuePage.addPattern(testPattern) you must wait until the
pattern is persisted and rendered before calling addPattern again; update the
test to poll the backend endpoint (e.g., /server/autoqueue/get) or use a
UI-ready helper such as waitForStream or a new
AutoQueuePage.waitForRow(testPattern) to ensure the first create completed, then
call AutoQueuePage.addPattern(testPattern) a second time and assert
AutoQueuePage.getErrorMessage() is visible.
In `@src/e2e-playwright/tests/dashboard.spec.ts`:
- Around line 103-110: The test "clicking a file row shows action buttons"
incorrectly queries row.locator(".actions button") — the dashboard renders
action controls as <div class="button"> elements with a click-guard pattern —
and the queue check uses a weak typeof assertion; update the selector in that
test (and the similar block at 113-123) to target ".actions .button" (or the
exact ".button" pattern used by dashboard.getFileRows()/action strip) and
replace the loose queue assertion so you only assert queue visibility after
selecting a row known to be queueable (or remove the redundant queue check).
Locate these changes in the test named "clicking a file row shows action
buttons", the use of dashboard.getFileRows(), and the variable/expectation named
queueVisible to apply the fix.
In `@src/e2e-playwright/tests/logs.spec.ts`:
- Around line 62-70: The current poll asserting records.count() <= count doesn't
verify the filter; change the expect.poll usage after typing into
logs.searchInput to poll a deterministic post-filter condition using records and
searchTerm: repeatedly fetch visible record texts (via
records.nth(i).textContent() or records.allTextContents()) and assert every
returned text includes the searchTerm (trimmed/lowercased if needed) so the
assertion only passes once the UI has actually filtered to matching items; keep
using expect.poll for auto-retry and reference records, logs.searchInput,
searchTerm, and expect.poll when locating the change.
In `@src/e2e-playwright/tests/pages/dashboard.page.ts`:
- Around line 60-63: getActionButton currently queries for a <button> inside the
file row but the UI renders actions as <div class="button">, so update
getActionButton (the method using fileRow.locator(".actions button", ...)) to
target the actual control (e.g., fileRow.locator(".actions .button", { hasText:
new RegExp(action, "i") })) so the helper resolves queue/stop/delete controls;
keep the case‑insensitive hasText RegExp and preserve existing signature and
return type.
In `@src/e2e-playwright/tests/pages/path-pairs.page.ts`:
- Around line 33-52: The fillForm function currently accepts enabled and
autoQueue but only fills three text inputs; either remove those properties or
wire them to the actual checkbox controls. Update fillForm (function name) to
locate the checkboxes inside the .pair-form locator (e.g.,
form.locator('input[type="checkbox"]') or form.getByRole('checkbox')) and for
fields.enabled and fields.autoQueue check whether each is defined, then query
the current state (isChecked) and call the appropriate action (check or uncheck)
to match the provided boolean; alternatively, if you choose not to support them,
remove enabled and autoQueue from the method signature so callers won’t expect
the page object to toggle those checkboxes.
In `@src/e2e-playwright/tests/path-pairs.spec.ts`:
- Around line 91-102: The POST setup requests in tests (e.g., inside the
"duplicate name shows error message" test using apiFetch) do not check the
response, causing silent failures later; update these fixtures to fail fast by
capturing the response from apiFetch, assert that res.ok (or res.status is in
the expected range), and throw or call test.fail/expect on non-OK responses;
factor this into a small helper (e.g., createFixture or postJsonHelper) that
sends JSON with headers, checks res.ok, and returns parsed JSON so all setup
calls (the POSTs currently using apiFetch across the file) can reuse it and
immediately surface creation errors.
- Around line 7-17: The current test.beforeEach unconditionally deletes all
path-pair records via apiFetch("/server/pathpairs"), which removes preexisting
data; instead capture the baseline set of IDs before tests run (e.g., in
test.beforeAll or at the start of test.beforeEach) by fetching
"/server/pathpairs" and storing their ids, then in test.afterEach only delete
path-pairs created by the test (either by tracking created IDs returned from
your test helpers or by deleting any ids from the fetched list that are not in
the baseline). Update uses of apiFetch and the deletion loop (the call to
apiFetch(`/server/pathpairs/${pair.id}`, { method: "DELETE" })) to consult the
baseline set and only issue DELETE for IDs that are test-created/non-baseline;
also ensure the same approach is mirrored for the block referenced in
test.afterEach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c65c424-9b0a-4334-b0e3-cfb57147fad9
📒 Files selected for processing (15)
.github/workflows/ci.ymlsrc/e2e-playwright/.gitignoresrc/e2e-playwright/tests/autoqueue.spec.tssrc/e2e-playwright/tests/dashboard.spec.tssrc/e2e-playwright/tests/fixtures.tssrc/e2e-playwright/tests/logs.spec.tssrc/e2e-playwright/tests/pages/about.page.tssrc/e2e-playwright/tests/pages/autoqueue.page.tssrc/e2e-playwright/tests/pages/dashboard.page.tssrc/e2e-playwright/tests/pages/logs.page.tssrc/e2e-playwright/tests/pages/path-pairs.page.tssrc/e2e-playwright/tests/pages/settings.page.tssrc/e2e-playwright/tests/path-pairs.spec.tssrc/e2e-playwright/tests/settings.spec.tstest-results/.last-run.json
Comment on lines
+1
to
+4
| node_modules/ | ||
| test-results/ | ||
| playwright-report/ | ||
| test-results/ |
There was a problem hiding this comment.
Move these Playwright artifact ignores to the repository root.
This nested .gitignore only covers src/e2e-playwright/**, so it does not stop root-level artifacts like the tracked test-results/.last-run.json from being committed. Git ignore rules are resolved relative to the .gitignore file location, and adding an ignore rule later does not untrack files that are already in the index. Please add these rules at the repo root (or make the suite always emit under src/e2e-playwright) and drop the duplicate test-results/ entry here. (git-scm.com)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e-playwright/.gitignore` around lines 1 - 4, Move the Playwright ignore
rules (node_modules/, playwright-report/, test-results/) out of this nested
.gitignore and add them to the repository root .gitignore (or change the test
output location so artifacts are always emitted under the e2e suite); remove the
duplicate test-results/ entry from this file so root-level artifacts like
test-results/.last-run.json are ignored and avoid tracking already-indexed files
by updating the root .gitignore and untracking any currently committed artifacts
with git rm --cached if needed.
Comment on lines
+13
to
+14
| /** Helper to wait for the SSE stream to connect and deliver initial data */ | ||
| waitForStream: (page: Page) => Promise<void>; |
There was a problem hiding this comment.
waitForStream currently waits for shell chrome, not stream readiness.
This helper only waits for a[href="/dashboard"], which the page objects already wait for in goto(). Callers then use it as if backend state has arrived, so checks like the dashboard fileCount can run against pre-stream UI and get skipped or flake. Wait on a page-specific loaded signal instead, or rename this helper so it is not treated as synchronization.
Also applies to: 26-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e-playwright/tests/fixtures.ts` around lines 13 - 14, The helper
waitForStream currently only waits for the shell chrome (the
a[href="/dashboard"] anchor) which duplicates Page.goto and does not guarantee
SSE/backend data arrival; update waitForStream (or rename it) to wait for a
page-specific "stream ready" signal instead — e.g. wait for a dashboard-specific
DOM change or test hook that the SSE has populated (such as the dashboard file
count element becoming visible/updated or a dedicated data-test="stream-ready"
marker) or await the actual SSE/endpoint response, so callers that rely on
backend state (like fileCount checks) run against a fully populated UI; if you
cannot add such a signal, rename waitForStream to reflect that it only waits for
chrome (e.g., waitForShellChrome) and update callers accordingly.
Comment on lines
+36
to
+51
| async addPattern(pattern: string) { | ||
| // Wait for input to be enabled (SSE stream must have delivered config) | ||
| await this.patternInput.waitFor({ state: "visible", timeout: 10_000 }); | ||
| // Wait until the input is not disabled (config has been received) | ||
| await this.page.waitForFunction( | ||
| () => { | ||
| const el = document.querySelector("input[type='search']") as HTMLInputElement; | ||
| return el && !el.disabled; | ||
| }, | ||
| { timeout: 10_000 } | ||
| ); | ||
| await this.patternInput.fill(pattern); | ||
| // Small delay for Angular change detection to process the input value | ||
| await this.page.waitForTimeout(200); | ||
| // Click with force since it's a div, not a native button | ||
| await this.addButton.click({ force: true }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/e2e-playwright/tests/pages/autoqueue.page.ts | head -60Repository: nitrobass24/seedsync
Length of output: 2403
🏁 Script executed:
# Find the button definition in the template
fd . --name "*.html" --type f | xargs grep -l "add-pattern" | head -5Repository: nitrobass24/seedsync
Length of output: 296
🏁 Script executed:
# Check if there are other test helpers using similar wait patterns for div buttons
rg "waitForFunction|waitForTimeout" src/e2e-playwright/tests/pages/ -A 2 -B 2Repository: nitrobass24/seedsync
Length of output: 1111
🏁 Script executed:
fd --type f '\.html$' | xargs grep -l "add-pattern"Repository: nitrobass24/seedsync
Length of output: 129
🏁 Script executed:
# Search for the add-pattern button definition
rg "add-pattern|#add-pattern" --type html -A 5 -B 2Repository: nitrobass24/seedsync
Length of output: 866
Synchronize on the button's disabled state instead of using a fixed sleep and forced click.
addPattern() relies on a 200ms sleep followed by a forced click to the div button, which can race the button's disabled state on slower runners. Since this codebase uses <div class="button"> controls with [attr.disabled] and click-guard patterns (as the button is here), the test should wait for the button's disabled attribute to be cleared before clicking, rather than sleeping for an arbitrary duration. Playwright discourages page.waitForTimeout() in production tests and recommends waiting on concrete signals instead.
♻️ Suggested change
async addPattern(pattern: string) {
// Wait for input to be enabled (SSE stream must have delivered config)
await this.patternInput.waitFor({ state: "visible", timeout: 10_000 });
// Wait until the input is not disabled (config has been received)
await this.page.waitForFunction(
() => {
const el = document.querySelector("input[type='search']") as HTMLInputElement;
return el && !el.disabled;
},
{ timeout: 10_000 }
);
await this.patternInput.fill(pattern);
- // Small delay for Angular change detection to process the input value
- await this.page.waitForTimeout(200);
- // Click with force since it's a div, not a native button
- await this.addButton.click({ force: true });
+ // Wait for button to be enabled (disabled attribute cleared)
+ await this.page.waitForFunction(
+ () => {
+ const button = document.querySelector("#add-pattern .button");
+ return button !== null && !button.hasAttribute("disabled");
+ },
+ { timeout: 5_000 }
+ );
+ await this.addButton.click();📝 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
| async addPattern(pattern: string) { | |
| // Wait for input to be enabled (SSE stream must have delivered config) | |
| await this.patternInput.waitFor({ state: "visible", timeout: 10_000 }); | |
| // Wait until the input is not disabled (config has been received) | |
| await this.page.waitForFunction( | |
| () => { | |
| const el = document.querySelector("input[type='search']") as HTMLInputElement; | |
| return el && !el.disabled; | |
| }, | |
| { timeout: 10_000 } | |
| ); | |
| await this.patternInput.fill(pattern); | |
| // Small delay for Angular change detection to process the input value | |
| await this.page.waitForTimeout(200); | |
| // Click with force since it's a div, not a native button | |
| await this.addButton.click({ force: true }); | |
| async addPattern(pattern: string) { | |
| // Wait for input to be enabled (SSE stream must have delivered config) | |
| await this.patternInput.waitFor({ state: "visible", timeout: 10_000 }); | |
| // Wait until the input is not disabled (config has been received) | |
| await this.page.waitForFunction( | |
| () => { | |
| const el = document.querySelector("input[type='search']") as HTMLInputElement; | |
| return el && !el.disabled; | |
| }, | |
| { timeout: 10_000 } | |
| ); | |
| await this.patternInput.fill(pattern); | |
| // Wait for button to be enabled (disabled attribute cleared) | |
| await this.page.waitForFunction( | |
| () => { | |
| const button = document.querySelector("#add-pattern .button"); | |
| return button !== null && !button.hasAttribute("disabled"); | |
| }, | |
| { timeout: 5_000 } | |
| ); | |
| await this.addButton.click(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e-playwright/tests/pages/autoqueue.page.ts` around lines 36 - 51, The
addPattern method currently uses a fixed 200ms wait and a forced click which can
race the button state; update addPattern (referencing addPattern, patternInput,
and addButton) to remove the waitForTimeout and instead wait until the
addButton's disabled attribute is cleared (or its [attr.disabled] is absent)
using a Playwright wait (e.g., waitForFunction or waitForSelector checking the
element is enabled) and then perform a normal click without { force: true };
this ensures the test synchronizes on the real clickable state of the div button
rather than a sleep.
Comment on lines
+35
to
+50
| test("text field change saves to backend", async ({ page, apiGet }) => { | ||
| const field = settings.getTextInput("Server Address"); | ||
| await field.clear(); | ||
| const testValue = "e2e-test-server"; | ||
| await field.fill(testValue); | ||
|
|
||
| // Poll the API until the value is saved | ||
| await expect | ||
| .poll( | ||
| async () => { | ||
| const config = await apiGet("/server/config/get"); | ||
| return config.lftp.remote_address; | ||
| }, | ||
| { timeout: 5000 } | ||
| ) | ||
| .toBe(testValue); |
There was a problem hiding this comment.
Make these persistence checks state-independent and self-cleaning.
If remote_address already equals "e2e-test-server" or log_format is already "json", the poll passes without verifying a save. The two Server Address tests also leave the mutated value behind. Read the current value first, pick a different target, and restore the original in a finally block for the address cases; the log-format test already has the restore half, it just needs a non-current target.
Also applies to: 90-122, 189-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e-playwright/tests/settings.spec.ts` around lines 35 - 50, The
persistence tests use fixed targets and can pass spuriously or leave state
mutated; update the "text field change saves to backend" test (using
settings.getTextInput, apiGet and expect.poll) to first read the current
remote_address via apiGet("/server/config/get"), choose a different testValue
when the current equals "e2e-test-server", then perform the
field.clear()/field.fill() and poll as before, and finally restore the original
value in a finally block (using api POST/PUT same endpoint or the UI flow you
already use) to ensure cleanup; apply the same pattern to the other Server
Address test blocks (lines referenced around 90-122 and 189-197) and change the
log_format test to pick a non-current target before toggling while keeping its
restore step.
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.
Summary
Replace the broken Protractor E2E suite with a modern Playwright-based test suite. Covers all 5 application pages with 66 tests using Page Object pattern.
Test coverage
Test approach
GET /server/config/getStructure
Removed
src/e2e/— old Protractor suite (non-functional since 2022)Makefile targets
make test-e2e— headlessmake test-e2e-headed— with browser visiblemake test-e2e-report— HTML reportCloses #250
Test plan
npx playwright test --listshows 66 tests🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores
Revert