feat: e2e tests with playwright#3
Conversation
Research from frappe/lms Cypress setup adapted for Playwright: - Directory structure and configuration - Auth, Frappe API, and workflow test helpers - Page Object Models for workflow editor - Test specifications for auth, CRUD, and editor - Complete CI/CD workflow for GitHub Actions
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a Playwright-based end-to-end testing suite and CI: documentation, Playwright config, helpers, page objects, tests, npm scripts, .gitignore entries, and a GitHub Actions workflow to run UI tests against a local Hazelnode/Frappe stack with persisted auth state and CSRF support. Changes
Sequence Diagram(s)sequenceDiagram
actor Developer
participant GH_Actions as GitHub Actions
participant Runner as CI Runner (ubuntu)
participant Services as MariaDB & Redis
participant Bench as Frappe Bench / Hazelnode
participant Playwright as Playwright Tests (Browser/API)
Developer->>GH_Actions: push / pull_request / workflow_dispatch
GH_Actions->>Runner: start job
Runner->>Services: start MariaDB & Redis (containers)
Runner->>Runner: install Python, Node, deps, MariaDB client
Runner->>Bench: setup bench, install Hazelnode app, provision site
Runner->>Bench: start Frappe server (background)
Playwright->>Bench: run API setup (auth.setup: login, save CSRF/storage)
Playwright->>Bench: run browser tests against BASE_URL (UI interactions)
Playwright->>Bench: perform API-backed setup/teardown (create/delete fixtures)
Playwright->>Runner: upload `playwright-report` & `test-results` artifacts
GH_Actions->>Developer: surface results & artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 |
|
|
||
| ### CI/CD Setup (`.github/workflows/ui-tests.yml`) | ||
|
|
||
| - **Trigger**: Runs on PRs, push to main, and manual dispatch |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @E2E_TESTS_PLAN.md:
- Around line 121-122: The plan references global-setup and global-teardown but
lacks documentation; add a short section describing what each helper must do and
why (e.g., global-setup: initialize test DB, apply migrations, create test
users, seed data, start test services; global-teardown: clean database state,
remove test users, stop services, clear temp files), and include a small
annotated example or checklist showing the expected exported functions
(setup/teardown) and error handling so implementers know to export async
functions that return/throw appropriately; ensure you reference the module names
global-setup and global-teardown and list key responsibilities like migrations,
seeding, and cleanup.
- Around line 52-76: The fenced code block showing the directory tree starting
with "hazelnode/" lacks a language identifier; update the opening fence from ```
to a language tag such as ```text (or ```bash) so the block becomes ```text and
the rest of the directory listing remains unchanged to satisfy Markdown linters
and renderers.
- Around line 375-378: The test.beforeAll hook that calls loginViaAPI and sets
workflowName via createTestWorkflow needs its scope clarified: decide whether it
should run once for the entire file or once per describe block; if it should be
global, move the test.beforeAll to the top-level (file scope) so it executes
once per worker for the file, otherwise move it inside the appropriate
test.describe block so it runs once per describe; ensure the hook still assigns
workflowName in the same way and that dependent tests reference that variable
appropriately.
- Around line 83-84: Update the vulnerable dependency declaration for
"@playwright/test" from "^1.49.0" to at least "^1.55.1" (preferably "^1.57.0");
update the dependency entry where "@playwright/test" appears, run your package
manager to refresh node_modules and the lockfile (npm/yarn/pnpm install), and
verify by running the test suite or a playwright install step to ensure the
updated version installs correctly.
🧹 Nitpick comments (3)
E2E_TESTS_PLAN.md (3)
278-280: Consider a more robust wait strategy for API responses.The response wait at line 278–280 uses a broad pattern (
resp.url().includes('/api/')) that may match multiple requests. Add specificity by matching the exact endpoint or method to avoid race conditions where an earlier request completes successfully but a later one fails silently.Suggested improvement:
async save() { await this.saveButton.click(); await this.page.waitForResponse( (resp) => resp.url().includes('/api/resource/Hazel Workflow') && resp.status() === 200 ); }
237-286: Add null-safety checks for page object locators.The
WorkflowEditorPageconstructor usespage.locator()directly without defensive checks. If a selector doesn't exist in the DOM at test runtime, locators will fail silently until an action is attempted. Consider documenting the expected DOM structure or adding inline comments about requireddata-testidattributes.Example enhancement (optional):
constructor(page: Page) { this.page = page; this.canvas = page.locator('.react-flow'); // Assumes react-flow library in use this.nodePanel = page.locator('[data-testid="node-panel"]'); // Requires frontend implementation // ... rest of locators }Verify that the frontend codebase includes
.react-flowcontainer and[data-testid]attributes as documented in Phase 6.5.
506-532: Verify Bench setup steps are compatible with current Frappe/Hazelnode versions.The Bench initialization, site setup, and app installation steps (lines 506–532) assume specific Frappe ecosystem versions and CLI behavior. Since your deployment environment may differ from the development context, test this workflow in a staging CI environment before merging to ensure:
bench initflags work as documented- Database initialization completes without timeouts
- Site setup with custom hostnames succeeds
- App installation and build steps are idempotent
Would you like me to generate a standalone shell script to validate the Bench setup steps locally before committing to CI?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
E2E_TESTS_PLAN.md
🧰 Additional context used
🪛 LanguageTool
E2E_TESTS_PLAN.md
[uncategorized] ~9-~9: The official name of this software platform is spelled with a capital “H”.
Context: ...dings from frappe/lms ### CI/CD Setup (.github/workflows/ui-tests.yml) - Trigger...
(GITHUB)
[uncategorized] ~439-~439: The official name of this software platform is spelled with a capital “H”.
Context: ... Integration #### 5.1 GitHub Workflow (.github/workflows/ui-tests.yml) ```yaml name:...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
E2E_TESTS_PLAN.md
52-52: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (5)
E2E_TESTS_PLAN.md (5)
241-249: Approve configuration of page object selectors.The use of semantic locators (
button:has-text("Save")) anddata-testidattributes follows modern Playwright best practices and aligns well with the frontend framework. This approach is robust and maintainable.
94-124: Approve Playwright configuration structure.The
playwright.config.tsfollows Playwright best practices:
- Sequential execution (
workers: 1) for Frappe state consistency ✓- CI-aware reporter selection ✓
- Video/screenshot/trace on failure for debugging ✓
- Proper use of environment variables for base URL ✓
Configuration is well-suited for the constraints of Frappe testing environments.
130-158: Approve authentication helper design.The dual-path authentication (API and UI) with sensible defaults (
'Administrator','admin') is pragmatic. API-first login avoids slow UI waits, while UI login tests verify the actual login flow. Thelogout()helper correctly uses the Frappe API endpoint.
202-227: Approve workflow helper interfaces and setup/teardown pattern.The
WorkflowNodeinterface and CRUD helpers provide a clean API for test data management. API-backed setup/teardown ensures idempotency and avoids UI interaction brittleness.
479-479: No action required. Python 3.14.2 is the current stable release as of January 2026 (released December 5, 2025), making the specification on line 479 accurate and appropriate.
| ``` | ||
| hazelnode/ | ||
| ├── e2e/ # Playwright test directory | ||
| │ ├── fixtures/ # Test data (workflows, images) | ||
| │ ├── helpers/ # Test utilities | ||
| │ │ ├── auth.ts # Login helpers | ||
| │ │ ├── workflow.ts # Workflow creation helpers | ||
| │ │ └── frappe.ts # Frappe API utilities | ||
| │ ├── pages/ # Page Object Models | ||
| │ │ ├── login.page.ts # Login page | ||
| │ │ ├── workflow-list.page.ts # Workflow list page | ||
| │ │ └── workflow-editor.page.ts # Workflow editor page | ||
| │ └── tests/ # Test specifications | ||
| │ ├── auth.spec.ts # Authentication tests | ||
| │ ├── workflow-crud.spec.ts # Workflow CRUD operations | ||
| │ └── workflow-editor.spec.ts # Editor interactions | ||
| ├── playwright.config.ts # Playwright configuration | ||
| └── .github/ | ||
| ├── workflows/ | ||
| │ └── ui-tests.yml # New UI tests workflow | ||
| └── helper/ | ||
| ├── install_dependencies.sh # System dependencies | ||
| ├── install.sh # Bench/site setup | ||
| └── site_config.json # Test site configuration | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
The directory structure code block is missing a language specifier. Markdown linters and documentation renderers expect all fenced blocks to declare their format.
- ```
+ ```
hazelnode/
├── e2e/ # Playwright test directory🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
52-52: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @E2E_TESTS_PLAN.md around lines 52 - 76, The fenced code block showing the
directory tree starting with "hazelnode/" lacks a language identifier; update
the opening fence from ``` to a language tag such as ```text (or ```bash) so the
block becomes ```text and the rest of the directory listing remains unchanged to
satisfy Markdown linters and renderers.
| "@playwright/test": "^1.49.0", | ||
| "@types/node": "^22.0.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest version of Playwright @playwright/test January 2026
💡 Result:
The latest stable @playwright/test release as of January 11, 2026 is v1.57.0 (release tag v1.57.0). [1][2]
Sources:
- Playwright GitHub releases (v1.57.0). [1]
- Playwright docs / installation (shows update instructions to @playwright/test@latest). [2]
🌐 Web query:
latest version of @types/node January 2026
💡 Result:
Latest published @types/node (as of January 11, 2026) is 24.3.1. [1][2]
Sources:
[1] npm package page for @types/node.
[2] DefinitelyTyped repository (types/node).
🌐 Web query:
Playwright 1.49.0 security vulnerabilities CVE 2025 2026
💡 Result:
Short answer: Yes — Playwright 1.49.0 is affected by at least one public CVE-class vulnerability from 2025 and is implicated in others (some affect related components/services). Key items:
-
CVE‑2025‑59288 — Improper verification of cryptographic signature in Playwright’s browser install scripts (uses curl -k). Affects Playwright versions < 1.55.1 (so 1.49.0 is vulnerable). Fixed in 1.55.1. [1][2]
-
CVE‑2025‑26683 — Improper authorization in Azure Playwright (cloud/runner environment). This is an Azure/runner issue (published Mar 31, 2025); if you use Azure Playwright integrations, you should apply Microsoft’s patch. [3]
-
CVE‑2025‑9611 — Origin-header validation / DNS-rebinding issue in Microsoft Playwright MCP Server (published Jan 7, 2026). Affects MCP server versions prior to 0.0.40 (fix: upgrade MCP server). (Not a direct playwright-core npm version number, but relevant if you run the MCP server component). [4][5]
Recommendations: upgrade playwright/playwright-core to a patched release (CVE‑59288 fixed in 1.55.1), apply Microsoft/Azure patches for Azure Playwright and MCP server (see advisories), and follow vendor release notes/SECURITY advisories. [1][2][3][4]
Sources:
- Snyk security entry for playwright-core (improper signature verification; fixed in 1.55.1).
- Wiz vulnerability summary / analysis for CVE‑2025‑59288.
- CVE summary / advisories for CVE‑2025‑26683 (Azure Playwright).
- Tenable / CVE entries for CVE‑2025‑9611 (MCP Server).
(Links: Snyk, Wiz, cve.news/feedly, Tenable / NVD entries above.)
Upgrade @playwright/test to address CVE-2025-59288.
@playwright/test@1.49.0 is vulnerable to CVE-2025-59288 (improper verification of cryptographic signatures in browser install scripts), fixed in v1.55.1. Upgrade to at least ^1.55.1 or higher (latest available is v1.57.0 as of January 2026).
@types/node@^22.0.0 is acceptable; no known vulnerabilities reported for this version range.
🤖 Prompt for AI Agents
In @E2E_TESTS_PLAN.md around lines 83 - 84, Update the vulnerable dependency
declaration for "@playwright/test" from "^1.49.0" to at least "^1.55.1"
(preferably "^1.57.0"); update the dependency entry where "@playwright/test"
appears, run your package manager to refresh node_modules and the lockfile
(npm/yarn/pnpm install), and verify by running the test suite or a playwright
install step to ensure the updated version installs correctly.
| globalSetup: require.resolve('./e2e/helpers/global-setup'), | ||
| globalTeardown: require.resolve('./e2e/helpers/global-teardown'), |
There was a problem hiding this comment.
Document the missing global setup and teardown helpers.
Lines 121-122 reference global-setup and global-teardown modules that are not described in the implementation plan. Add brief documentation or a code snippet explaining what these files should contain (e.g., database cleanup, test user creation, site teardown).
Would you like me to generate template implementations for global-setup.ts and global-teardown.ts?
🤖 Prompt for AI Agents
In @E2E_TESTS_PLAN.md around lines 121 - 122, The plan references global-setup
and global-teardown but lacks documentation; add a short section describing what
each helper must do and why (e.g., global-setup: initialize test DB, apply
migrations, create test users, seed data, start test services; global-teardown:
clean database state, remove test users, stop services, clear temp files), and
include a small annotated example or checklist showing the expected exported
functions (setup/teardown) and error handling so implementers know to export
async functions that return/throw appropriately; ensure you reference the module
names global-setup and global-teardown and list key responsibilities like
migrations, seeding, and cleanup.
| test.beforeAll(async ({ request }) => { | ||
| await loginViaAPI(request); | ||
| workflowName = await createTestWorkflow(request, 'Editor Test Workflow'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Playwright test.beforeAll test.beforeEach describe block scoping behavior
💡 Result:
Short summary — scoping & order for test.beforeAll / test.beforeEach in Playwright:
-
Scope:
- Hooks apply to the scope where they are declared: top-level (the file) or inside a test.describe() group. A beforeAll/beforeEach declared inside a describe runs only for tests in that describe (and its nested describes). [1][2]
-
Frequency / worker behavior:
- test.beforeAll runs once per scope per worker process (i.e., once for the file or describe in that worker). It can run again if Playwright reloads the file (for example on retries or worker restarts). [1][2][5]
-
Execution order when nesting:
- For beforeAll: outer describe beforeAll runs before inner describe beforeAll.
- For beforeEach: outer beforeEach runs before inner beforeEach, and both run before the test body.
- afterEach/afterAll run in the reverse order. [1][2]
-
Fixtures interaction:
- Fixtures follow their own scopes (test vs worker) and are set up/teared down according to dependency and scope rules; beforeEach hooks run after test-scoped fixtures are set up. Use worker-scoped fixtures for global beforeAll-like behavior across files. [2]
If you want, I can show a small example illustrating nested describe hook order and the retry/file-reload behavior.
Clarify intended scope for test.beforeAll().
In Playwright, test.beforeAll() runs once per scope per worker: if declared inside a test.describe() block, it runs once per describe block; if declared at file level, it runs once for the entire file. Verify whether this hook is intended to run once across all file tests or once per describe block, and move it to the appropriate scope if needed.
🤖 Prompt for AI Agents
In @E2E_TESTS_PLAN.md around lines 375 - 378, The test.beforeAll hook that calls
loginViaAPI and sets workflowName via createTestWorkflow needs its scope
clarified: decide whether it should run once for the entire file or once per
describe block; if it should be global, move the test.beforeAll to the top-level
(file scope) so it executes once per worker for the file, otherwise move it
inside the appropriate test.describe block so it runs once per describe; ensure
the hook still assigns workflowName in the same way and that dependent tests
reference that variable appropriately.
Add complete e2e testing setup using Playwright following best practices: - Add Playwright dependencies and test scripts to package.json - Configure Playwright with setup project pattern for authentication - Create test helpers for Frappe API, authentication, and workflow operations - Implement Page Object Models for Login, WorkflowList, and WorkflowEditor - Add test specifications for auth, workflow CRUD, and editor interactions - Create GitHub Actions workflow for automated UI testing Key features: - Uses Playwright's recommended "setup project" pattern for auth state reuse - Pre-authenticated tests via storageState for faster execution - Fresh state tests for authentication flow testing - Proper cleanup with try/finally blocks for test isolation - CI workflow with Frappe bench setup and Playwright browser caching Directory structure: e2e/ ├── helpers/ # Auth, Frappe API, workflow utilities ├── pages/ # Page Object Models ├── tests/ # Test specifications └── tsconfig.json
npm ci requires package-lock.json which doesn't exist in root directory
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @e2e/helpers/auth.ts:
- Around line 28-45: The loginViaUI function duplicates selector and flow logic
from the LoginPage object; replace its manual steps by constructing and using
LoginPage (e.g., new LoginPage(page)) and calling its login(email, password) so
selectors and click/fill logic are centralized; also update the post-login wait
to use the same URL pattern as LoginPage.login() (include /hazelnode/ in the
regex) and remove the duplicated selectors/fills in loginViaUI.
In @e2e/pages/workflow-editor.page.ts:
- Around line 106-127: In dragNodeToCanvas, the calculated viewport-relative
targetPosition is unused causing dragTo to use incorrect coordinates; either
remove the unused calculation or pass the computed targetPosition to
paletteNode.dragTo. Update the call in dragNodeToCanvas (function) to await
paletteNode.dragTo(this.canvas, { targetPosition }) so it uses
canvasBounds-adjusted coordinates, or if original targetX/targetY are already
correct, delete the targetPosition block to avoid confusion and keep
paletteNode.dragTo as-is.
In @e2e/pages/workflow-list.page.ts:
- Around line 122-128: The workflowExists function builds a locator using string
interpolation which risks selector injection via the title; change it to use
Playwright's safer API (e.g., this.page.locator('table tr', { hasText: title }))
instead of template literals so the title is passed as a separate hasText
argument (locate the workflowExists method and the row locator variable to
update).
- Around line 99-105: The clickWorkflow method interpolates title into a CSS
selector which can break with quotes/backslashes; fix by not injecting raw text
into the selector—either (A) update the UI to add a stable data attribute (e.g.,
data-testid like workflow-row-<sanitizedName>) on each <tr> and change
clickWorkflow to locate by that data-testid (use the same sanitization logic)
then click and waitForURL, or (B) use Playwright's hasText option to avoid
manual escaping: locate the row via this.page.locator('table tr', { hasText:
title }).click(); keep the waitForURL call unchanged.
- Around line 107-120: The code interpolates title into a CSS/text selector
causing selector-injection if title contains special chars; in
toggleWorkflowEnabled replace the template literal locator with Playwright's
safer text-matching API (e.g., use this.page.locator('table tr').filter({
hasText: title }) or this.page.locator('table tr', { hasText: title }) and then
find the switch via row.getByRole('switch') or
row.locator('button[role="switch"]'), keeping the same waitForResponse logic;
update references in toggleWorkflowEnabled accordingly.
- Around line 138-143: The selector currently interpolates the title into a
string (`this.page.locator(\`table tr:has-text("${title}")\`)`) which risks
selector injection; change it to use Playwright's locator API with a separate
hasText option to avoid interpolation, e.g., replace the locator call in
expectWorkflowVisible with this.page.locator('table tr', { hasText: title })
(optionally .first() if multiple matches) so the title is passed as data rather
than injected into the selector string.
🧹 Nitpick comments (15)
e2e/pages/workflow-list.page.ts (1)
54-64: Consider adding a timeout parameter to waitForLoaded.The hardcoded 30-second timeout might be too long for fast CI environments or too short for slow ones. Making it configurable would improve flexibility.
♻️ Proposed enhancement
- async waitForLoaded(): Promise<void> { + async waitForLoaded(timeout: number = 30000): Promise<void> { // Wait for loading state to finish await this.page.waitForLoadState('networkidle'); // Wait for either the table or error message await Promise.race([ - this.workflowTable.waitFor({ state: 'visible', timeout: 30000 }), - this.errorMessage.waitFor({ state: 'visible', timeout: 30000 }), + this.workflowTable.waitFor({ state: 'visible', timeout }), + this.errorMessage.waitFor({ state: 'visible', timeout }), ]).catch(() => { // May already be visible }); }e2e/pages/workflow-editor.page.ts (2)
33-56: Consider adding data-testid attributes for more stable selectors.Several locators rely on CSS classes, text content, or attribute patterns that may be fragile:
- Line 34:
nodePaletteuses[class*="scroll-area"], .p-3:has(h3)which couples to implementation details- Line 47-52:
actionNodesandlogicNodesuse:has-text()which breaks if UI copy changes- Line 56:
saveButtonuses text matching instead of a stable identifierWhile these selectors may work initially, they create maintenance burden when UI evolves.
♻️ Recommended approach: Add data-testid attributes
In your React components, add stable test identifiers:
// In the component <div data-testid="node-palette">...</div> <button data-testid="workflow-save-button">Save</button> <div data-testid="action-nodes-section">...</div>Then update the page object:
-this.nodePalette = page.locator('[class*="scroll-area"], .p-3:has(h3)'); +this.nodePalette = page.locator('[data-testid="node-palette"]'); -this.saveButton = page.locator('button:has-text("Save")'); +this.saveButton = page.locator('[data-testid="workflow-save-button"]'); -this.actionNodes = page.locator('h4:has-text("Actions") + div [draggable="true"]'); +this.actionNodes = page.locator('[data-testid="action-nodes-section"] [draggable="true"]');
247-255: Handle potential null fromgetAttribute.Line 251 calls
getAttribute('data-id')which can returnnull. Although there's a null check on line 252, the type narrowing may not be sufficient if the attribute is missing.♻️ More defensive null handling
async getAllNodeIds(): Promise<string[]> { const nodes = await this.nodes.all(); const ids: string[] = []; for (const node of nodes) { const id = await node.getAttribute('data-id'); - if (id) ids.push(id); + if (id !== null && id !== '') ids.push(id); } return ids; }Or use a more functional approach:
async getAllNodeIds(): Promise<string[]> { const nodes = await this.nodes.all(); - const ids: string[] = []; - for (const node of nodes) { - const id = await node.getAttribute('data-id'); - if (id) ids.push(id); - } - return ids; + return (await Promise.all( + nodes.map(node => node.getAttribute('data-id')) + )).filter((id): id is string => id !== null); }playwright.config.ts (1)
38-53: Consider adding Firefox and WebKit projects for cross-browser testing.Currently, only Chromium is configured. Playwright supports Firefox and WebKit (Safari) out of the box. Adding these would improve test coverage across browsers.
♻️ Add Firefox and WebKit projects
projects: [ // Setup project - authenticates and saves state { name: 'setup', testMatch: /auth\.setup\.ts/, }, // Main test project - uses stored auth state { name: 'chromium', use: { ...devices['Desktop Chrome'], storageState: authFile, }, dependencies: ['setup'], }, + { + name: 'firefox', + use: { + ...devices['Desktop Firefox'], + storageState: authFile, + }, + dependencies: ['setup'], + }, + { + name: 'webkit', + use: { + ...devices['Desktop Safari'], + storageState: authFile, + }, + dependencies: ['setup'], + }, ],e2e/tests/workflow-crud.spec.ts (3)
23-42: Wrap cleanup in try/finally for test isolation.The cleanup code at line 41 will not execute if an assertion fails. This could leave orphaned test data. For consistency with other tests and better isolation, wrap the test logic in a try/finally block.
♻️ Proposed refactor
test('should create a new workflow via UI', async ({ page, request }) => { const listPage = new WorkflowListPage(page); const workflowTitle = generateWorkflowTitle('UI Create'); - await listPage.goto(); - await listPage.createWorkflow(workflowTitle); - - // Should navigate to editor - await expect(page).toHaveURL(/.*workflow\/.*/); - - // Verify workflow was created via API - const urlParts = page.url().split('/'); - const workflowId = urlParts[urlParts.length - 1]; - - const workflow = await getWorkflow(request, workflowId); - expect(workflow.title).toBe(workflowTitle); - - // Cleanup - await deleteTestWorkflow(request, workflowId); + let workflowId: string | undefined; + + try { + await listPage.goto(); + await listPage.createWorkflow(workflowTitle); + + // Should navigate to editor + await expect(page).toHaveURL(/.*workflow\/.*/); + + // Verify workflow was created via API + const urlParts = page.url().split('/'); + workflowId = urlParts[urlParts.length - 1]; + + const workflow = await getWorkflow(request, workflowId); + expect(workflow.title).toBe(workflowTitle); + } finally { + // Cleanup + if (workflowId) { + await deleteTestWorkflow(request, workflowId); + } + } });
74-75: Escape regex variable to prevent potential ReDoS.Constructing a RegExp from a variable without escaping can be problematic. While
workflowNameis controlled in tests, escaping is a best practice.♻️ Proposed refactor
Option 1: Use string matching instead of regex:
- // Should navigate to editor - await expect(page).toHaveURL(new RegExp(`.*workflow/${workflowName}.*`)); + // Should navigate to editor + expect(page.url()).toContain(`/workflow/${workflowName}`);Option 2: Escape the variable:
- // Should navigate to editor - await expect(page).toHaveURL(new RegExp(`.*workflow/${workflowName}.*`)); + // Should navigate to editor + const escapedName = workflowName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + await expect(page).toHaveURL(new RegExp(`.*workflow/${escapedName}.*`));Based on static analysis hints.
82-109: Consider verifying the initial state before toggling.The test assumes the initial state is false (line 97 comment), but doesn't verify it. This could lead to false positives if the workflow creation logic changes.
♻️ Proposed enhancement
try { await listPage.goto(); + + // Verify initial state + const initialWorkflow = await getWorkflow(request, workflowName); + expect(initialWorkflow.is_active).toBeFalsy(); // Toggle enabled state await listPage.toggleWorkflowEnabled(workflowTitle);e2e/tests/auth.spec.ts (1)
42-51: Consider verifying error message visibility.The test verifies that the user stays on the login page after invalid credentials, but doesn't verify that an error message is actually shown. The
LoginPageclass provides anexpectLoginError()method that could be used here.♻️ Proposed enhancement
test('should show error for invalid credentials', async ({ page }) => { const loginPage = new LoginPage(page); await loginPage.goto(); await loginPage.fillCredentials('invalid@example.com', 'wrongpassword'); await loginPage.submit(); // Should stay on login page await loginPage.expectToBeOnLoginPage(); + // Should show error message + await loginPage.expectLoginError(); });e2e/tests/workflow-editor.spec.ts (6)
31-51: Replace hard wait with condition-based waiting.Line 43 uses
page.waitForTimeout(1000)which can make tests flaky or unnecessarily slow. Consider waiting for a specific condition instead, such as the presence of node elements.♻️ Proposed refactor
const editorPage = new WorkflowEditorPage(page); await editorPage.goto(workflowName); - // Wait for nodes to render - await page.waitForTimeout(1000); + // Wait for nodes to render + await page.locator('.react-flow__node').first().waitFor({ state: 'visible' }); // Should have at least one node (the webhook trigger) const nodeCount = await editorPage.getNodeCount();
53-76: Replace hard wait with condition-based waiting.Line 68 uses
page.waitForTimeout(1000)which can make tests flaky or unnecessarily slow. Consider waiting for a specific condition instead, such as the presence of edge elements.♻️ Proposed refactor
const editorPage = new WorkflowEditorPage(page); await editorPage.goto(workflowName); - // Wait for edges to render - await page.waitForTimeout(1000); + // Wait for edges to render + await page.locator('.react-flow__edge').first().waitFor({ state: 'visible' }); // Should have at least one edge connecting the nodes const edgeCount = await editorPage.getEdgeCount();
112-135: Replace hard wait with condition-based waiting.Line 121 uses
page.waitForTimeout(1000)which can make tests flaky. Consider waiting for the first node to be visible before attempting to select it.♻️ Proposed refactor
const editorPage = new WorkflowEditorPage(page); await editorPage.goto(workflowName); - // Wait for nodes to render - await page.waitForTimeout(1000); - // Get first node and click it const nodeIds = await editorPage.getAllNodeIds(); expect(nodeIds.length).toBeGreaterThan(0); + + // Wait for node to be visible + const firstNode = editorPage.getNodeById(nodeIds[0]); + await firstNode.waitFor({ state: 'visible' }); await editorPage.selectNode(nodeIds[0]);
161-194: Replace hard waits with condition-based waiting.Lines 176 and 185 use
page.waitForTimeout(1000)which can make tests flaky. Consider waiting for nodes to be visible after loading and after reload.♻️ Proposed refactor
const editorPage = new WorkflowEditorPage(page); await editorPage.goto(workflowName); - // Wait for nodes to render - await page.waitForTimeout(1000); + // Wait for nodes to render + await page.locator('.react-flow__node').first().waitFor({ state: 'visible' }); const initialNodeCount = await editorPage.getNodeCount(); // Reload the page await page.reload(); await editorPage.waitForEditorReady(); - // Wait for nodes to render again - await page.waitForTimeout(1000); + // Wait for nodes to render again + await page.locator('.react-flow__node').first().waitFor({ state: 'visible' }); const reloadedNodeCount = await editorPage.getNodeCount();
202-228: Replace hard wait with condition-based waiting.Line 219 uses
page.waitForTimeout(500)after drag-and-drop. Consider waiting for the node count to change or for the new node to appear.♻️ Proposed refactor
// Try to drag a Log node to the canvas await editorPage.dragNodeToCanvas('Log', 300, 200); - // Wait for node to be added - await page.waitForTimeout(500); - const newNodeCount = await editorPage.getNodeCount(); // Should have one more node expect(newNodeCount).toBe(initialNodeCount + 1);If the count check is not reliable immediately after drag, you could add:
+ // Wait for node count to update + await expect.poll(async () => await editorPage.getNodeCount()).toBe(initialNodeCount + 1);
230-259: Replace hard waits with condition-based waiting.Lines 239 and 249 use
page.waitForTimeout()which can make tests flaky. Consider waiting for specific conditions.♻️ Proposed refactor
const editorPage = new WorkflowEditorPage(page); await editorPage.goto(workflowName); - // Wait for nodes to render - await page.waitForTimeout(1000); + // Wait for nodes to render + await page.locator('.react-flow__node').first().waitFor({ state: 'visible' }); const initialNodeCount = await editorPage.getNodeCount(); const nodeIds = await editorPage.getAllNodeIds(); if (nodeIds.length > 0) { // Select and delete the first node await editorPage.deleteNode(nodeIds[0]); - // Wait for deletion - await page.waitForTimeout(500); - const newNodeCount = await editorPage.getNodeCount(); // Should have one less node expect(newNodeCount).toBe(initialNodeCount - 1);Or use polling:
+ // Wait for node count to update + await expect.poll(async () => await editorPage.getNodeCount()).toBe(initialNodeCount - 1);e2e/helpers/workflow.ts (1)
144-160: Consider addinggetListto static imports.The dynamic import of
getListat Line 151 works correctly but is inconsistent with the static imports at Line 2. For consistency and clarity, consider addinggetListto the static imports alongside the other frappe helpers.♻️ Suggested refactor
-import { createDoc, deleteDoc, getDoc, docExists } from './frappe'; +import { createDoc, deleteDoc, getDoc, docExists, getList } from './frappe';Then simplify Line 151:
- const { getList } = await import('./frappe'); - const workflows = await getList<HazelWorkflow>(request, 'Hazel Workflow', { + const workflows = await getList<HazelWorkflow>(request, 'Hazel Workflow', {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/ui-tests.yml.gitignoree2e/helpers/auth.tse2e/helpers/frappe.tse2e/helpers/index.tse2e/helpers/workflow.tse2e/pages/index.tse2e/pages/login.page.tse2e/pages/workflow-editor.page.tse2e/pages/workflow-list.page.tse2e/tests/auth.setup.tse2e/tests/auth.spec.tse2e/tests/workflow-crud.spec.tse2e/tests/workflow-editor.spec.tse2e/tsconfig.jsonpackage.jsonplaywright.config.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: BuildWithHussain/hazelnode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:50:59.725Z
Learning: Run tests using: bench --site test_site run-tests --app hazelnode
📚 Learning: 2026-01-11T16:50:59.725Z
Learnt from: CR
Repo: BuildWithHussain/hazelnode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:50:59.725Z
Learning: Build frontend React app to ../hazelnode/public/frontend/ directory
Applied to files:
.gitignore
📚 Learning: 2026-01-11T16:50:59.724Z
Learnt from: CR
Repo: BuildWithHussain/hazelnode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:50:59.724Z
Learning: Applies to frontend/src/routes/**/*.tsx : Frontend basepath should be /hazelnode for all routes and asset references
Applied to files:
.gitignore
📚 Learning: 2026-01-11T16:50:59.724Z
Learnt from: CR
Repo: BuildWithHussain/hazelnode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:50:59.724Z
Learning: Applies to frontend/**/*.{ts,tsx} : Use ESLint and Prettier for TypeScript linting and formatting
Applied to files:
e2e/tsconfig.json
📚 Learning: 2026-01-11T16:50:59.724Z
Learnt from: CR
Repo: BuildWithHussain/hazelnode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:50:59.724Z
Learning: Applies to frontend/**/*.{ts,tsx} : Enable strict mode for TypeScript
Applied to files:
e2e/tsconfig.json
📚 Learning: 2026-01-11T16:50:59.725Z
Learnt from: CR
Repo: BuildWithHussain/hazelnode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:50:59.725Z
Learning: Run tests using: bench --site test_site run-tests --app hazelnode
Applied to files:
.github/workflows/ui-tests.yml
🧬 Code graph analysis (6)
e2e/pages/login.page.ts (1)
e2e/pages/index.ts (1)
LoginPage(1-1)
e2e/pages/workflow-list.page.ts (1)
e2e/pages/index.ts (1)
WorkflowListPage(2-2)
e2e/tests/auth.spec.ts (2)
e2e/helpers/auth.ts (1)
isLoggedIn(72-86)e2e/pages/login.page.ts (1)
LoginPage(6-70)
e2e/tests/workflow-editor.spec.ts (2)
e2e/helpers/workflow.ts (5)
generateWorkflowTitle(138-142)createTestWorkflow(41-58)deleteTestWorkflow(63-70)createWebhookWorkflow(85-99)createWebhookLogWorkflow(104-133)e2e/pages/index.ts (1)
WorkflowEditorPage(3-3)
e2e/helpers/workflow.ts (1)
e2e/helpers/frappe.ts (5)
createDoc(16-32)docExists(161-172)deleteDoc(81-94)getDoc(37-53)getList(120-156)
e2e/pages/workflow-editor.page.ts (1)
e2e/pages/index.ts (1)
WorkflowEditorPage(3-3)
🪛 ast-grep (0.40.4)
e2e/tests/workflow-crud.spec.ts
[warning] 74-74: 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(.*workflow/${workflowName}.*)
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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Server
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (41)
.gitignore (1)
11-15: LGTM! Standard Playwright ignore patterns.The added entries correctly exclude Playwright-generated artifacts and sensitive authentication state from version control.
e2e/tsconfig.json (1)
1-25: LGTM! Well-configured TypeScript setup for e2e tests.The configuration correctly enables strict mode (aligning with project learnings), uses modern module resolution, and defines helpful path aliases that will streamline imports across the test suite.
package.json (2)
11-14: LGTM! Comprehensive Playwright test scripts.The four scripts provide good coverage for different testing scenarios: headless CI runs, interactive UI mode, headed debugging, and step-by-step debugging.
19-22: Both dependency versions are valid and stable.
@playwright/test@1.49.0is published and stable. With the caret constraint, the latest compatible version (1.57.0) will be installed.
@types/node@22.0.0is published and stable, though note that@types/nodehas progressed to v25.0.3. Consider updating the constraint to^25.0.0if compatibility with the latest type definitions is needed..github/workflows/ui-tests.yml (3)
113-126: LGTM! Robust server startup with proper readiness check.The server startup logic correctly disables resource-intensive processes (watch/schedule), starts the server in the background with logging, and uses a timeout-protected readiness check before proceeding to tests.
133-138: LGTM! Properly configured test execution.The environment variables correctly point to the local Frappe instance with appropriate credentials for the test run.
46-50: Node.js 24 has been released and is the Active LTS version as of January 2026 (latest release: 24.12.0 from December 10, 2025). The workflow configuration is correct and requires no changes. Usingcheck-latest: trueensures the most recent 24.x patch is used, which is appropriate for CI/CD workflows.Likely an incorrect or invalid review comment.
e2e/pages/workflow-list.page.ts (2)
21-41: LGTM! Well-organized page object structure.The constructor properly initializes all necessary locators for the workflow list page and create dialog, providing a clean interface for test interactions.
46-64: LGTM! Robust page loading strategy.The combination of
networkidleand racing between success/error states ensures reliable test execution regardless of the page outcome.e2e/pages/workflow-editor.page.ts (2)
182-190: LGTM! Robust save implementation.The save method properly waits for the specific PUT response with URL encoding, ensuring the workflow is persisted before continuing. This prevents race conditions in tests.
70-73: The current implementation is appropriate for this codebase. A search of the codebase found no websockets, long-polling, or background API patterns that would interfere withnetworkidle. The combination of waiting for the network idle state and then verifying canvas visibility is a sound approach for this application.Likely an incorrect or invalid review comment.
e2e/helpers/index.ts (1)
1-3: LGTM! Clean barrel export pattern.This provides a convenient single import point for all helper modules.
e2e/tests/auth.setup.ts (3)
38-39: Same networkidle concern as workflow editor.Similar to the workflow editor page object,
networkidlemay not be reliable if the Frappe app uses websockets, long-polling, or continuous background requests.Based on learnings, you might want to align the wait strategy across all page objects and setup files.
15-44: LGTM! Follows Playwright's recommended setup project pattern.The authentication flow is well-structured:
- API-based login (faster than UI)
- Verification step ensures login succeeded
- Navigation to /app ensures cookies are properly set
- Storage state persisted for reuse
17-22: The fallback credentials are appropriate for the test environment.The
Administrator/admindefaults match what the CI workflows create (seeui-tests.ymlline 100 andci.ymlline 90), so these are not a security risk—the E2E tests are designed to run against ephemeral test sites with those credentials. However, for clarity and explicit CI/CD configuration, consider settingFRAPPE_USERandFRAPPE_PASSWORDenvironment variables in the workflow instead of relying on fallbacks.e2e/pages/index.ts (1)
1-3: LGTM! Clean barrel export for page objects.Provides a convenient single import point for all page object models.
playwright.config.ts (2)
16-54: LGTM! Well-configured Playwright setup.The configuration properly implements the setup project pattern, enforces sequential execution for stateful backend consistency, and includes appropriate timeouts and retry policies.
30-30: The.testdomain setup is already documented inE2E_TESTS_PLAN.mdwith explicit instructions for hosts file configuration (127.0.0.1 hazelnode.test) and comprehensive Frappe bench setup steps. No action needed.Likely an incorrect or invalid review comment.
e2e/tests/workflow-crud.spec.ts (3)
16-21: LGTM!Clean test structure for verifying the workflow list page loads correctly.
44-58: LGTM!Proper use of try/finally ensures test isolation even if assertions fail.
111-152: LGTM!Both tests demonstrate good practices:
- Dialog interaction test verifies visibility states correctly
- API creation test uses proper try/finally cleanup pattern
e2e/pages/login.page.ts (4)
1-19: LGTM!Good use of data attributes for form field selectors, which are more stable than CSS classes. The error message locator appropriately covers multiple possible error containers.
21-27: LGTM!The
networkidlewait strategy is appropriate for a login page to ensure all resources are loaded before interaction.
44-55: LGTM!The login flow is well-structured. The 30-second timeout for post-login navigation is conservative but appropriate for E2E tests where network conditions may vary. The URL pattern
/(app|desk|hazelnode)/properly covers the expected post-login destinations.
57-69: LGTM!Clear assertion methods that provide good test readability.
e2e/tests/auth.spec.ts (4)
8-23: LGTM!Clean tests that verify the pre-authenticated state setup works correctly. The combination of UI navigation and API verification provides good coverage.
29-31: LGTM!Good use of
test.use()to reset storage state for testing unauthenticated scenarios.
33-40: LGTM!Clean test that verifies successful UI login flow.
53-60: LGTM!Properly verifies that unauthenticated users are redirected to login when accessing protected pages.
e2e/tests/workflow-editor.spec.ts (3)
17-29: LGTM!Clean test structure with proper cleanup using try/finally.
78-110: LGTM!Both tests have clean structure and appropriate assertions without hard waits.
137-159: LGTM!Good test coverage of zoom controls with proper cleanup.
e2e/helpers/auth.ts (2)
5-26: LGTM!Good implementation of API-based login with proper error handling. Using form data is correct for Frappe's login endpoint.
47-86: LGTM!All helper functions are well-implemented:
logout()uses the correct API endpointsaveAuthState()andgetStorageStatePath()provide centralized storage managementisLoggedIn()has appropriate error handling and correctly interprets the API responsee2e/helpers/frappe.ts (4)
1-11: LGTM!The
FrappeResponseinterface and imports are well-structured. The optional fields align with Frappe's API response patterns.
13-94: LGTM!The CRUD operations are well-implemented with:
- Proper URL encoding via
encodeURIComponentfor document names- Consistent error handling pattern
- Type-safe generic return types
- Appropriate HTTP methods for each operation
117-172: LGTM!The
getListfunction properly constructs query parameters with JSON serialization for complex values (fields, filters). ThedocExistshelper elegantly uses exception handling to determine document existence without exposing errors to callers.
96-115: The implementation correctly follows Frappe API specifications. ThecallMethodfunction properly returnsresult.message, which aligns with Frappe's/api/method/endpoint design, while CRUD operations correctly useresult.datafor/api/resource/endpoints. This difference reflects Frappe's API structure where RPC method calls and REST resource operations have distinct response formats. The code is correct.e2e/helpers/workflow.ts (3)
1-35: LGTM!The workflow-related interfaces are well-structured with appropriate required and optional fields that model the Hazel Workflow domain entities effectively.
37-133: LGTM!The workflow helper functions demonstrate good practices:
deleteTestWorkflowdefensively checks existence before deletion- Factory functions (
createWebhookWorkflow,createWebhookLogWorkflow) provide useful test fixtures with sensible defaults- The API is clean and type-safe
135-142: LGTM!The
generateWorkflowTitlefunction effectively generates unique identifiers using timestamp and random suffix, preventing test data collisions.
| /** | ||
| * Login via UI (for testing the login flow itself). | ||
| */ | ||
| export async function loginViaUI( | ||
| page: Page, | ||
| email: string = 'Administrator', | ||
| password: string = 'admin' | ||
| ): Promise<void> { | ||
| await page.goto('/login'); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| await page.fill('input[data-fieldname="email"]', email); | ||
| await page.fill('input[data-fieldname="password"]', password); | ||
| await page.click('button[type="submit"]'); | ||
|
|
||
| // Wait for redirect to desk/app | ||
| await page.waitForURL(/\/(app|desk)/, { timeout: 30000 }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor to use LoginPage to avoid duplication.
This function duplicates the selectors and login logic from LoginPage (e2e/pages/login.page.ts). Additionally, the URL pattern /(app|desk)/ at line 44 is inconsistent with LoginPage.login() which uses /(app|desk|hazelnode)/.
Using the page object would eliminate duplication and ensure consistency.
♻️ Proposed refactor
+import { LoginPage } from '../pages/login.page';
+
/**
* Login via UI (for testing the login flow itself).
*/
export async function loginViaUI(
page: Page,
email: string = 'Administrator',
password: string = 'admin'
): Promise<void> {
- await page.goto('/login');
- await page.waitForLoadState('networkidle');
-
- await page.fill('input[data-fieldname="email"]', email);
- await page.fill('input[data-fieldname="password"]', password);
- await page.click('button[type="submit"]');
-
- // Wait for redirect to desk/app
- await page.waitForURL(/\/(app|desk)/, { timeout: 30000 });
+ const loginPage = new LoginPage(page);
+ await loginPage.login(email, password);
}📝 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.
| /** | |
| * Login via UI (for testing the login flow itself). | |
| */ | |
| export async function loginViaUI( | |
| page: Page, | |
| email: string = 'Administrator', | |
| password: string = 'admin' | |
| ): Promise<void> { | |
| await page.goto('/login'); | |
| await page.waitForLoadState('networkidle'); | |
| await page.fill('input[data-fieldname="email"]', email); | |
| await page.fill('input[data-fieldname="password"]', password); | |
| await page.click('button[type="submit"]'); | |
| // Wait for redirect to desk/app | |
| await page.waitForURL(/\/(app|desk)/, { timeout: 30000 }); | |
| } | |
| import { LoginPage } from '../pages/login.page'; | |
| /** | |
| * Login via UI (for testing the login flow itself). | |
| */ | |
| export async function loginViaUI( | |
| page: Page, | |
| email: string = 'Administrator', | |
| password: string = 'admin' | |
| ): Promise<void> { | |
| const loginPage = new LoginPage(page); | |
| await loginPage.login(email, password); | |
| } |
🤖 Prompt for AI Agents
In @e2e/helpers/auth.ts around lines 28 - 45, The loginViaUI function duplicates
selector and flow logic from the LoginPage object; replace its manual steps by
constructing and using LoginPage (e.g., new LoginPage(page)) and calling its
login(email, password) so selectors and click/fill logic are centralized; also
update the post-login wait to use the same URL pattern as LoginPage.login()
(include /hazelnode/ in the regex) and remove the duplicated selectors/fills in
loginViaUI.
| /** | ||
| * Click on a workflow by its title. | ||
| */ | ||
| async clickWorkflow(title: string): Promise<void> { | ||
| await this.page.locator(`table tr:has-text("${title}")`).click(); | ||
| await this.page.waitForURL(/.*workflow\/.*/, { timeout: 30000 }); | ||
| } |
There was a problem hiding this comment.
Potential selector injection when title contains special characters.
The title parameter is directly interpolated into a selector string without escaping. If a workflow title contains quotes, backslashes, or other special characters, the selector will fail or behave unexpectedly.
🔒 Proposed fix using data-testid attributes
In your React component rendering workflow rows, add:
<tr data-testid={`workflow-row-${workflow.name}`}>Then update the method:
async clickWorkflow(title: string): Promise<void> {
- await this.page.locator(`table tr:has-text("${title}")`).click();
+ // Use getByRole or more specific selector
+ await this.page.locator(`table tr`, { hasText: title }).click();
await this.page.waitForURL(/.*workflow\/.*/, { timeout: 30000 });
}Alternatively, use Playwright's built-in hasText option which handles escaping automatically.
🤖 Prompt for AI Agents
In @e2e/pages/workflow-list.page.ts around lines 99 - 105, The clickWorkflow
method interpolates title into a CSS selector which can break with
quotes/backslashes; fix by not injecting raw text into the selector—either (A)
update the UI to add a stable data attribute (e.g., data-testid like
workflow-row-<sanitizedName>) on each <tr> and change clickWorkflow to locate by
that data-testid (use the same sanitization logic) then click and waitForURL, or
(B) use Playwright's hasText option to avoid manual escaping: locate the row via
this.page.locator('table tr', { hasText: title }).click(); keep the waitForURL
call unchanged.
| /** | ||
| * Toggle the enabled state of a workflow. | ||
| */ | ||
| async toggleWorkflowEnabled(title: string): Promise<void> { | ||
| const row = this.page.locator(`table tr:has-text("${title}")`); | ||
| const toggle = row.locator('button[role="switch"]'); | ||
| await toggle.click(); | ||
| // Wait for API response | ||
| await this.page.waitForResponse( | ||
| (resp) => | ||
| resp.url().includes('/api/resource/Hazel%20Workflow') && | ||
| resp.request().method() === 'PUT' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Same selector injection risk in toggleWorkflowEnabled.
The title parameter is interpolated without escaping, which could break if the title contains special characters.
🔒 Proposed fix
async toggleWorkflowEnabled(title: string): Promise<void> {
- const row = this.page.locator(`table tr:has-text("${title}")`);
+ const row = this.page.locator(`table tr`, { hasText: title });
const toggle = row.locator('button[role="switch"]');
await toggle.click();
// Wait for API response
await this.page.waitForResponse(
(resp) =>
resp.url().includes('/api/resource/Hazel%20Workflow') &&
resp.request().method() === 'PUT'
);
}📝 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.
| /** | |
| * Toggle the enabled state of a workflow. | |
| */ | |
| async toggleWorkflowEnabled(title: string): Promise<void> { | |
| const row = this.page.locator(`table tr:has-text("${title}")`); | |
| const toggle = row.locator('button[role="switch"]'); | |
| await toggle.click(); | |
| // Wait for API response | |
| await this.page.waitForResponse( | |
| (resp) => | |
| resp.url().includes('/api/resource/Hazel%20Workflow') && | |
| resp.request().method() === 'PUT' | |
| ); | |
| } | |
| /** | |
| * Toggle the enabled state of a workflow. | |
| */ | |
| async toggleWorkflowEnabled(title: string): Promise<void> { | |
| const row = this.page.locator(`table tr`, { hasText: title }); | |
| const toggle = row.locator('button[role="switch"]'); | |
| await toggle.click(); | |
| // Wait for API response | |
| await this.page.waitForResponse( | |
| (resp) => | |
| resp.url().includes('/api/resource/Hazel%20Workflow') && | |
| resp.request().method() === 'PUT' | |
| ); | |
| } |
🤖 Prompt for AI Agents
In @e2e/pages/workflow-list.page.ts around lines 107 - 120, The code
interpolates title into a CSS/text selector causing selector-injection if title
contains special chars; in toggleWorkflowEnabled replace the template literal
locator with Playwright's safer text-matching API (e.g., use
this.page.locator('table tr').filter({ hasText: title }) or
this.page.locator('table tr', { hasText: title }) and then find the switch via
row.getByRole('switch') or row.locator('button[role="switch"]'), keeping the
same waitForResponse logic; update references in toggleWorkflowEnabled
accordingly.
| /** | ||
| * Check if a workflow exists in the list. | ||
| */ | ||
| async workflowExists(title: string): Promise<boolean> { | ||
| const row = this.page.locator(`table tr:has-text("${title}")`); | ||
| return (await row.count()) > 0; | ||
| } |
There was a problem hiding this comment.
Same selector injection risk in workflowExists.
🔒 Proposed fix
async workflowExists(title: string): Promise<boolean> {
- const row = this.page.locator(`table tr:has-text("${title}")`);
+ const row = this.page.locator(`table tr`, { hasText: title });
return (await row.count()) > 0;
}🤖 Prompt for AI Agents
In @e2e/pages/workflow-list.page.ts around lines 122 - 128, The workflowExists
function builds a locator using string interpolation which risks selector
injection via the title; change it to use Playwright's safer API (e.g.,
this.page.locator('table tr', { hasText: title })) instead of template literals
so the title is passed as a separate hasText argument (locate the workflowExists
method and the row locator variable to update).
| /** | ||
| * Assert that a workflow is visible in the list. | ||
| */ | ||
| async expectWorkflowVisible(title: string): Promise<void> { | ||
| await expect(this.page.locator(`table tr:has-text("${title}")`)).toBeVisible(); | ||
| } |
There was a problem hiding this comment.
Same selector injection risk in expectWorkflowVisible.
🔒 Proposed fix
async expectWorkflowVisible(title: string): Promise<void> {
- await expect(this.page.locator(`table tr:has-text("${title}")`)).toBeVisible();
+ await expect(this.page.locator(`table tr`, { hasText: title })).toBeVisible();
}📝 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.
| /** | |
| * Assert that a workflow is visible in the list. | |
| */ | |
| async expectWorkflowVisible(title: string): Promise<void> { | |
| await expect(this.page.locator(`table tr:has-text("${title}")`)).toBeVisible(); | |
| } | |
| /** | |
| * Assert that a workflow is visible in the list. | |
| */ | |
| async expectWorkflowVisible(title: string): Promise<void> { | |
| await expect(this.page.locator(`table tr`, { hasText: title })).toBeVisible(); | |
| } |
🤖 Prompt for AI Agents
In @e2e/pages/workflow-list.page.ts around lines 138 - 143, The selector
currently interpolates the title into a string (`this.page.locator(\`table
tr:has-text("${title}")\`)`) which risks selector injection; change it to use
Playwright's locator API with a separate hasText option to avoid interpolation,
e.g., replace the locator call in expectWorkflowVisible with
this.page.locator('table tr', { hasText: title }) (optionally .first() if
multiple matches) so the title is passed as data rather than injected into the
selector string.
- Update LoginPage selectors to match actual Frappe login HTML structure: - Use #login_email, autocomplete="username", name="usr" for email - Use #login_password for password - Use .btn-login for submit button - Fix auth.setup.ts to use page.request instead of separate request fixture - page.request shares cookies with browser context - Ensure .auth directory exists before saving state These changes ensure the authentication works correctly with Frappe's login page structure and properly persists cookies for subsequent tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
e2e/tests/auth.setup.ts (2)
27-28: Consider requiring environment variables instead of defaults.Hard-coded credential defaults could pose a security risk if this setup is accidentally run against a non-test environment. Consider failing explicitly if
FRAPPE_USERorFRAPPE_PASSWORDare not set.🔒 Proposed change to require explicit credentials
+ if (!process.env.FRAPPE_USER || !process.env.FRAPPE_PASSWORD) { + throw new Error('FRAPPE_USER and FRAPPE_PASSWORD environment variables must be set'); + } + const loginResponse = await page.request.post('/api/method/login', { form: { - usr: process.env.FRAPPE_USER || 'Administrator', - pwd: process.env.FRAPPE_PASSWORD || 'admin', + usr: process.env.FRAPPE_USER, + pwd: process.env.FRAPPE_PASSWORD, }, });
25-41: Consider adding descriptive error messages to assertions.The current assertions will fail if authentication doesn't work, but the error messages could be more helpful for debugging. Adding custom messages would make troubleshooting easier.
📝 Proposed improvement for error messages
- expect(loginResponse.ok()).toBeTruthy(); + expect(loginResponse.ok(), `Login request failed with status ${loginResponse.status()}`).toBeTruthy(); const userResponse = await page.request.get( '/api/method/frappe.auth.get_logged_user' ); - expect(userResponse.ok()).toBeTruthy(); + expect(userResponse.ok(), `Failed to fetch logged user: ${userResponse.status()}`).toBeTruthy(); const userData = await userResponse.json(); - expect(userData.message).not.toBe('Guest'); + expect(userData.message, 'User is still Guest - authentication failed').not.toBe('Guest');.github/workflows/ui-tests.yml (2)
76-82: Consider more specific cache key for Playwright browsers.The Playwright browser cache key uses
hashFiles('**/package.json'), which will invalidate whenever any package.json changes. Consider using a more specific key based on the Playwright version to avoid unnecessary cache invalidation.♻️ Proposed improvement for cache specificity
- name: Cache Playwright browsers uses: actions/cache@v4 with: path: ~/.cache/ms-playwright - key: ${{ runner.os }}-playwright-${{ hashFiles('**/package.json') }} + key: ${{ runner.os }}-playwright-${{ hashFiles('**/package-lock.json', '**/package.json') }} restore-keys: | ${{ runner.os }}-playwright-Note: This assumes package-lock.json will be added. Alternatively, extract the Playwright version specifically:
- name: Get Playwright Version id: playwright-version run: echo "version=$(node -p "require('./package.json').devDependencies['@playwright/test']")" >> $GITHUB_OUTPUT - name: Cache Playwright browsers uses: actions/cache@v4 with: path: ~/.cache/ms-playwright key: ${{ runner.os }}-playwright-${{ steps.playwright-version.outputs.version }} restore-keys: | ${{ runner.os }}-playwright-
128-131: Consider adding package-lock.json for deterministic builds.The workflow uses
npm installinstead ofnpm cibecause package-lock.json is absent. While this works, it can lead to non-deterministic builds if dependency versions are not strictly pinned in package.json.For more reliable CI builds, consider:
- Running
npm installlocally to generate package-lock.json- Committing package-lock.json to the repository
- Updating this step to use
npm cifor faster, deterministic installsThis ensures that all CI runs use identical dependency versions.
e2e/pages/login.page.ts (1)
78-83: Consider a more specific URL pattern.The regex
/.*login.*/is quite broad and could match unintended URLs containing "login" anywhere. Consider using a more specific pattern.♻️ Proposed improvement for URL matching
async expectToBeOnLoginPage(): Promise<void> { - await expect(this.page).toHaveURL(/.*login.*/); + await expect(this.page).toHaveURL(/\/login(?:\?.*)?$/); }This matches URLs ending with
/login(with optional query parameters), which is more precise.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ui-tests.ymle2e/pages/login.page.tse2e/tests/auth.setup.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: BuildWithHussain/hazelnode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:50:59.725Z
Learning: Run tests using: bench --site test_site run-tests --app hazelnode
📚 Learning: 2026-01-11T16:50:59.725Z
Learnt from: CR
Repo: BuildWithHussain/hazelnode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:50:59.725Z
Learning: Run tests using: bench --site test_site run-tests --app hazelnode
Applied to files:
.github/workflows/ui-tests.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Playwright E2E Tests
- GitHub Check: Server
🔇 Additional comments (9)
e2e/tests/auth.setup.ts (1)
1-52: LGTM! Solid authentication setup implementation.The setup script correctly implements Playwright's recommended pattern using
page.requestto share cookies with the browser context, ensures the auth directory exists, verifies login success, and saves the storage state for test reuse..github/workflows/ui-tests.yml (6)
1-12: LGTM! Well-configured workflow triggers and concurrency.The workflow triggers and concurrency settings are appropriate for a UI testing workflow, with good resource management via cancel-in-progress.
20-35: LGTM! Proper service configuration with health checks.The services are well-configured with appropriate health checks for MariaDB and correct port mappings for the Frappe environment.
84-106: LGTM! Proper Frappe bench setup.The bench initialization, database configuration, and app installation steps are correctly sequenced and configured for a Frappe environment.
113-126: LGTM! Robust server startup with health checking.The server startup approach with watch/schedule disabled, background execution, and health check timeout is well-designed for CI environments.
140-164: LGTM! Excellent artifact and log handling.The artifact uploads with
if: always()and failure log display are well-configured for debugging test failures.
41-50: No action needed. Python 3.14 and Node.js 24 are both stable and current as of January 2026. Python 3.14.0 was released October 7, 2025 (latest: 3.14.2), and Node.js 24 entered Active LTS on October 28, 2025 (latest: 24.12.0). These are appropriate versions for the workflow.Likely an incorrect or invalid review comment.
e2e/pages/login.page.ts (2)
11-33: LGTM! Well-designed locator strategy with fallbacks.The use of multiple selector options provides good compatibility across different Frappe versions. The selectors align with the commit message's mention of fixing them to match Frappe's actual HTML structure.
One minor consideration: Line 28 uses
button:has-text("Login")which could fail if Frappe is configured with a different language. Verify whether the test environment will always use English, or consider adding a data-testid attribute to the button if you have control over the Frappe frontend.
58-69: LGTM! Comprehensive login method with appropriate waits.The login method properly combines navigation, form filling, and submission with a post-login URL wait. The 30-second timeout and URL pattern match are appropriate for Frappe's redirect behavior.
Note: The default credentials match those in
e2e/tests/auth.setup.ts, which maintains consistency across the test suite. If you implement the suggestion to require explicit credentials in auth.setup.ts, consider applying the same approach here.
CSRF Token: - Add getCsrfToken() that fetches token from frappe.auth.get_csrf_token - Cache CSRF token per request context to avoid repeated calls - Include X-Frappe-CSRF-Token header in POST/PUT/DELETE requests Login Selectors: - Use exact Frappe login page IDs (#login_email, #login_password) - Use button.btn-login for submit button - Reference: frappe/frappe/www/login.html
Based on frappe/lms pattern: - auth.setup.ts now extracts window.frappe.csrf_token after login - Saves CSRF token to e2e/.auth/csrf.json - frappe.ts reads token from file instead of trying API endpoint This matches how frappe/lms handles CSRF: the token is passed via context.boot.csrf_token from the Python backend (lms/www/lms.py) and made available in window.frappe.csrf_token on the frontend.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @e2e/helpers/frappe.ts:
- Around line 17-18: The module-level csrfTokenCache variable can cause stale
tokens and race conditions in parallel tests; remove csrfTokenCache and change
the CSRF-reading logic so each request/test reads the token from disk (or make
the cache request-scoped by moving it into the function or accepting a
request/context object), updating any CSRF helper functions that reference
csrfTokenCache to load the token per-call (or use the provided context-scoped
cache) to ensure no shared state across tests.
- Around line 225-230: docExists currently swallows all errors from getDoc and
returns false, hiding real API failures; change the catch to inspect the caught
error from getDoc(request, doctype, name) (e.g., check error.status or
error.response?.status or specific NotFound/404 sentinel) and only return false
when the status indicates a 404/not-found; for any other error rethrow (or
propagate) the error so auth/network/server issues surface to callers. Ensure
you reference the existing docExists function and the getDoc(request, doctype,
name) call when making this change.
In @e2e/pages/login.page.ts:
- Around line 54-57: The login function currently hardcodes credentials in its
parameter defaults (login(email: string = 'Administrator', password: string =
'admin')), which is a security risk; change it to read credentials from
environment/config (e.g., use process.env.E2E_ADMIN_EMAIL and
process.env.E2E_ADMIN_PASSWORD or a secured config loader) and remove the
literal defaults, and add a clear error or fail-fast behavior if the env vars
are missing; also update the e2e test setup documentation to list the required
environment variables and how to provide them.
🧹 Nitpick comments (5)
e2e/pages/login.page.ts (2)
33-33: Consider replacingnetworkidlewith more specific wait conditions.The
networkidlewait state can be slow and flaky in CI environments. Consider waiting for specific elements to be visible instead, or usedomcontentloadedif the page structure allows.♻️ Suggested alternative wait strategy
async goto(): Promise<void> { await this.page.goto('/login'); - await this.page.waitForLoadState('networkidle'); + await this.page.waitForLoadState('domcontentloaded'); + // Wait for form to be ready + await this.emailInput.waitFor({ state: 'visible' }); }
25-25: Consider implications of selecting only the first error message.Using
.first()on the error message locator means only the first error will be checked. If multiple errors can appear simultaneously, this might miss important validation feedback.If multiple error messages are possible and relevant for testing, consider adding a method to check for specific error text or count of errors.
e2e/helpers/frappe.ts (3)
32-36: Use async file operations in async context.The synchronous
fs.readFileSyncandfs.existsSynccalls block the event loop. In a Node.js/Playwright context, prefer async file operations.♻️ Proposed refactor to async file operations
+import { promises as fsPromises } from 'fs'; -function getCsrfToken(): string { +async function getCsrfToken(): Promise<string> { // Read token from file try { - if (fs.existsSync(CSRF_FILE)) { - const data = JSON.parse(fs.readFileSync(CSRF_FILE, 'utf-8')); + try { + const content = await fsPromises.readFile(CSRF_FILE, 'utf-8'); + const data = JSON.parse(content); return data.csrf_token || ''; + } catch (err: any) { + if (err.code === 'ENOENT') { + return ''; + } + throw err; } } catch (error) { console.warn('Failed to read CSRF token file:', error); } return ''; }Note: This would require updating all callers to
await getCsrfToken().
68-69: Add runtime validation before accessing response properties.The code directly accesses
result.dataorresult.messagewithout verifying the response structure. If the API returns an unexpected format, this could cause runtime errors.Consider adding minimal validation:
const result = await response.json(); if (!result || typeof result !== 'object') { throw new Error(`Unexpected API response format`); } return result.data as T;This is particularly important given that the
FrappeResponseinterface marks all fields as optional.Also applies to: 90-90, 121-121, 173-173, 214-214
15-15: Consider usingpath.join(__dirname, ...)for more robust path resolution.The path
e2e/.auth/csrf.jsonworks correctly as-is since Playwright executes from the repository root (testDir only specifies where to find tests, not the execution directory). However, this makes the code dependent on the working directory context. Usingpath.join(__dirname, '../.auth/csrf.json')in bothauth.setup.tsandfrappe.tswould make the path resolution more explicit and independent of execution context, improving robustness if the setup ever changes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/helpers/frappe.tse2e/pages/login.page.tse2e/tests/auth.setup.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/tests/auth.setup.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: BuildWithHussain/hazelnode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:50:59.725Z
Learning: Run tests using: bench --site test_site run-tests --app hazelnode
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Playwright E2E Tests
- GitHub Check: Server
| // Cache for CSRF token (read from file once) | ||
| let csrfTokenCache: string | null = null; |
There was a problem hiding this comment.
Module-level CSRF token cache may cause issues in parallel test execution.
The module-level csrfTokenCache variable is shared across all tests running in the same worker. If tests run in parallel or if the CSRF token changes between test runs, this cache could serve stale tokens or cause race conditions.
♻️ Proposed fix: Remove caching or make it request-scoped
Consider removing the cache entirely since file reads are relatively fast, or implement request-context-scoped caching:
-// Cache for CSRF token (read from file once)
-let csrfTokenCache: string | null = null;
-
/**
* Get CSRF token from the file saved during auth setup.
* The token is extracted from window.frappe.csrf_token after login.
*/
function getCsrfToken(): string {
- // Return cached token if available
- if (csrfTokenCache !== null) {
- return csrfTokenCache;
- }
-
// Read token from file
try {
if (fs.existsSync(CSRF_FILE)) {
const data = JSON.parse(fs.readFileSync(CSRF_FILE, 'utf-8'));
- csrfTokenCache = data.csrf_token || '';
- return csrfTokenCache;
+ return data.csrf_token || '';
}
} catch (error) {
console.warn('Failed to read CSRF token file:', error);
}
- csrfTokenCache = '';
return '';
}Also applies to: 24-43
🤖 Prompt for AI Agents
In @e2e/helpers/frappe.ts around lines 17 - 18, The module-level csrfTokenCache
variable can cause stale tokens and race conditions in parallel tests; remove
csrfTokenCache and change the CSRF-reading logic so each request/test reads the
token from disk (or make the cache request-scoped by moving it into the function
or accepting a request/context object), updating any CSRF helper functions that
reference csrfTokenCache to load the token per-call (or use the provided
context-scoped cache) to ensure no shared state across tests.
| try { | ||
| await getDoc(request, doctype, name); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Error swallowing in docExists may hide real API issues.
The docExists function catches all errors and returns false, which means legitimate API errors (network issues, authentication failures, server errors) are silently suppressed and treated as "document doesn't exist."
🔍 Proposed improvement to distinguish 404 from other errors
export async function docExists(
request: APIRequestContext,
doctype: string,
name: string
): Promise<boolean> {
try {
await getDoc(request, doctype, name);
return true;
- } catch {
+ } catch (error: any) {
+ // Only treat 404 as "doesn't exist", rethrow other errors
+ if (error.message?.includes('404') || error.message?.includes('does not exist')) {
+ return false;
+ }
+ throw error;
- return false;
}
}This ensures real failures (auth issues, network errors, server errors) are properly surfaced rather than silently treated as missing documents.
📝 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.
| try { | |
| await getDoc(request, doctype, name); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| try { | |
| await getDoc(request, doctype, name); | |
| return true; | |
| } catch (error: any) { | |
| // Only treat 404 as "doesn't exist", rethrow other errors | |
| if (error.message?.includes('404') || error.message?.includes('does not exist')) { | |
| return false; | |
| } | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
In @e2e/helpers/frappe.ts around lines 225 - 230, docExists currently swallows
all errors from getDoc and returns false, hiding real API failures; change the
catch to inspect the caught error from getDoc(request, doctype, name) (e.g.,
check error.status or error.response?.status or specific NotFound/404 sentinel)
and only return false when the status indicates a 404/not-found; for any other
error rethrow (or propagate) the error so auth/network/server issues surface to
callers. Ensure you reference the existing docExists function and the
getDoc(request, doctype, name) call when making this change.
| async login( | ||
| email: string = 'Administrator', | ||
| password: string = 'admin' | ||
| ): Promise<void> { |
There was a problem hiding this comment.
Hardcoded credentials pose a security risk.
Default credentials Administrator / admin should not be hardcoded in source code. Consider using environment variables or a secure configuration file that's excluded from version control.
🔒 Proposed fix using environment variables
async login(
- email: string = 'Administrator',
- password: string = 'admin'
+ email: string = process.env.E2E_ADMIN_EMAIL || 'Administrator',
+ password: string = process.env.E2E_ADMIN_PASSWORD || 'admin'
): Promise<void> {Additionally, document the required environment variables in your e2e test setup documentation.
📝 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.
| async login( | |
| email: string = 'Administrator', | |
| password: string = 'admin' | |
| ): Promise<void> { | |
| async login( | |
| email: string = process.env.E2E_ADMIN_EMAIL || 'Administrator', | |
| password: string = process.env.E2E_ADMIN_PASSWORD || 'admin' | |
| ): Promise<void> { |
🤖 Prompt for AI Agents
In @e2e/pages/login.page.ts around lines 54 - 57, The login function currently
hardcodes credentials in its parameter defaults (login(email: string =
'Administrator', password: string = 'admin')), which is a security risk; change
it to read credentials from environment/config (e.g., use
process.env.E2E_ADMIN_EMAIL and process.env.E2E_ADMIN_PASSWORD or a secured
config loader) and remove the literal defaults, and add a clear error or
fail-fast behavior if the env vars are missing; also update the e2e test setup
documentation to list the required environment variables and how to provide
them.
- Changed `is_active` to `enabled` (matches doctype schema) - Changed `node_type` to `type` for nodes - Changed `source`/`target` to `source_node_id`/`target_node_id` for connections - Added `trigger_type` and `trigger_config` support in createTestWorkflow - Added helper functions for creating valid workflows: - createScheduleWorkflow: creates workflow with Schedule Event trigger - createScheduleLogWorkflow: workflow with trigger and log node - createDocumentEventWorkflow: workflow with Document Event trigger - Fixed test assertions to use `workflow.enabled` instead of `workflow.is_active`
- Make titleInput selector more flexible to handle id or type - Add explicit timeout for dialog visibility (HeadlessUI transitions) - Wait for titleInput after dialog opens - Use first() for titleInput in case of multiple matches
- Add domcontentloaded wait before networkidle - Wait for page title and button visibility during load - Ensure button is enabled before clicking to open dialog - Use first() for titleInput wait to handle multiple matches
Workflow List Page: - Use content-based dialog selector (dialog title text) instead of role="dialog" - HeadlessUI transitions can leave dialog in DOM but hidden - Increased dialog visibility timeout to 15s Workflow Editor Page: - Use exact text match for palette node names to avoid ambiguity - Wait for ReactFlow controls to be visible and enabled before clicking - Improve waitForEditorReady to include controls initialization - Wait for palette node visibility before drag operation
Zoom controls: - Use keyboard shortcuts (Ctrl+=/Ctrl+-) instead of clicking buttons - ReactFlow zoom buttons may have varying enabled states - Add getControlButton helper for flexible attribute matching Node deletion: - Click directly on node to ensure proper selection and focus - Verify node has 'selected' class before attempting delete - Try both Delete and Backspace keys for compatibility
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @e2e/pages/workflow-list.page.ts:
- Around line 136-139: The dynamic selector in workflowExists uses string
interpolation into a selector and can be exploited; replace the
`this.page.locator(\`table tr:has-text("${title}")\`)` usage with Playwright's
safer filter pattern by locating rows first (e.g., `this.page.locator('table
tr')`) and then applying `.filter({ hasText: title })` before counting, so the
title is treated as data rather than injected into the selector.
- Around line 152-154: The dynamic selector in expectWorkflowVisible(title) uses
string interpolation which can break or be exploited if title contains special
characters; change the locator to use Playwright's hasText option instead of
embedding the title into the selector (i.e., locate 'table tr' and pass {
hasText: title }) so the title is treated as text content rather than part of
the selector; update the expect call to use that locator and remove the
interpolated selector.
- Around line 121-131: The dynamic locator in toggleWorkflowEnabled interpolates
title directly into a CSS selector which breaks for special characters; update
toggleWorkflowEnabled to locate the row using a safe filter (e.g., use
this.page.locator('table tr').filter({ hasText: title }) or Playwright's
getByRole/getByText filtering) instead of `table tr:has-text("${title}")`, then
find the switch button via row.locator('button[role="switch"]') and proceed;
mirror the same change you applied to clickWorkflow so titles with special
characters are handled safely.
- Around line 113-116: The clickWorkflow method interpolates title directly into
a CSS selector which can break for titles containing quotes, parentheses,
backslashes, etc.; fix by either escaping special characters in title before
using it in page.locator(`table tr:has-text("${title}")`) or switch to a robust
locator pattern: first select the row elements (e.g., table tr) and then filter
by visible text using Playwright's has/hasText/filter APIs (refer to
clickWorkflow and the page.locator usage) so the dynamic title is matched safely
without injecting raw characters into the selector.
🧹 Nitpick comments (6)
e2e/tests/workflow-crud.spec.ts (1)
60-80: Consider escaping regex special characters for robustness.While the static analysis tool flagged the RegExp construction as a potential ReDoS vulnerability, the risk is minimal since
workflowNameis generated server-side by Frappe with a predictable format. However, escaping special regex characters would make the code more defensive.♻️ Optional refactor to escape regex characters
+/** + * Escape special regex characters in a string. + */ +function escapeRegex(str: string): string { + return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + test('should navigate to workflow editor when clicking a workflow', async ({ request, page, }) => { const listPage = new WorkflowListPage(page); const workflowTitle = generateWorkflowTitle('Click Test'); // Create workflow via API const workflowName = await createTestWorkflow(request, workflowTitle); try { await listPage.goto(); await listPage.clickWorkflow(workflowTitle); // Should navigate to editor - await expect(page).toHaveURL(new RegExp(`.*workflow/${workflowName}.*`)); + await expect(page).toHaveURL(new RegExp(`.*workflow/${escapeRegex(workflowName)}.*`)); } finally { // Cleanup await deleteTestWorkflow(request, workflowName); } });Alternatively, use a simpler string match:
- await expect(page).toHaveURL(new RegExp(`.*workflow/${workflowName}.*`)); + expect(page.url()).toContain(`/workflow/${workflowName}`);e2e/pages/workflow-editor.page.ts (5)
95-97: Consider escaping special characters in nodeId selector.The
nodeIdparameter is interpolated directly into the CSS selector. While ReactFlow node IDs are typically application-controlled, escaping special characters would make the selector more robust.♻️ Optional refactor to escape selector
getNodeById(nodeId: string): Locator { - return this.page.locator(`.react-flow__node[data-id="${nodeId}"]`); + // CSS.escape() escapes special characters in CSS selectors + const escapedId = nodeId.replace(/["\\]/g, '\\$&'); + return this.page.locator(`.react-flow__node[data-id="${escapedId}"]`); }
103-108: Consider escaping quotes in nodeName selector.The
nodeNameparameter is used in atext-isselector. While exact text matching is safer thanhas-text, escaping quotes would prevent potential selector breakage.♻️ Optional refactor
getPaletteNode(nodeName: string): Locator { // Match Card with draggable that contains a span with exact node name + const escapedName = nodeName.replace(/["']/g, '\\$&'); return this.page.locator( - `[draggable="true"]:has(span.font-medium:text-is("${nodeName}"))` + `[draggable="true"]:has(span.font-medium:text-is("${escapedName}"))` ); }
153-167: Consider escaping special characters in node and handle selectors.Multiple parameters are interpolated into CSS selectors without escaping. While these values are typically application-controlled, escaping would make the code more defensive.
♻️ Optional refactor to escape parameters
async connectNodes( sourceNodeId: string, targetNodeId: string, sourceHandle: string = 'source', targetHandle: string = 'target' ): Promise<void> { + const escapeAttr = (str: string) => str.replace(/["\\]/g, '\\$&'); const sourceConnector = this.page.locator( - `.react-flow__node[data-id="${sourceNodeId}"] .react-flow__handle[data-handleid="${sourceHandle}"]` + `.react-flow__node[data-id="${escapeAttr(sourceNodeId)}"] .react-flow__handle[data-handleid="${escapeAttr(sourceHandle)}"]` ); const targetConnector = this.page.locator( - `.react-flow__node[data-id="${targetNodeId}"] .react-flow__handle[data-handleid="${targetHandle}"]` + `.react-flow__node[data-id="${escapeAttr(targetNodeId)}"] .react-flow__handle[data-handleid="${escapeAttr(targetHandle)}"]` ); await sourceConnector.dragTo(targetConnector); }
182-192: Consider removing the hard-coded timeout.Line 189 uses a hard-coded 100ms delay. While the delay is small and follows verification, consider whether it's necessary given that the selection is already verified on line 187.
♻️ Optional refactor to remove timeout
If the 100ms delay proves unnecessary in practice:
async deleteNode(nodeId: string): Promise<void> { const node = this.getNodeById(nodeId); // Click on the node to select it and ensure focus await node.click(); // Verify node is selected await expect(node).toHaveClass(/selected/); - // Small delay to ensure selection is registered - await this.page.waitForTimeout(100); // Press delete keys await this.deleteSelectedNode(); }If the delay is necessary due to ReactFlow's internal state management, consider adding a comment explaining why.
228-244: LGTM! Good use of keyboard shortcuts for zoom.Using keyboard shortcuts is more reliable than clicking buttons. However, the JSDoc comments mention "Falls back to button click if available" but no fallback is implemented. Consider updating the comments to match the actual behavior.
📝 Optional: Update JSDoc to match implementation
/** * Zoom in on the canvas using keyboard shortcut. - * Falls back to button click if available. */ async zoomIn(): Promise<void> { await this.waitForControlsReady(); // Use keyboard shortcut: Ctrl/Cmd + Plus await this.canvas.click(); await this.page.keyboard.press('Control+='); } /** * Zoom out on the canvas using keyboard shortcut. - * Falls back to button click if available. */ async zoomOut(): Promise<void> { await this.waitForControlsReady(); // Use keyboard shortcut: Ctrl/Cmd + Minus await this.canvas.click(); await this.page.keyboard.press('Control+-'); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/helpers/workflow.tse2e/pages/workflow-editor.page.tse2e/pages/workflow-list.page.tse2e/tests/workflow-crud.spec.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: BuildWithHussain/hazelnode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:50:59.725Z
Learning: Run tests using: bench --site test_site run-tests --app hazelnode
🧬 Code graph analysis (1)
e2e/pages/workflow-list.page.ts (1)
e2e/pages/index.ts (1)
WorkflowListPage(2-2)
🪛 ast-grep (0.40.4)
e2e/tests/workflow-crud.spec.ts
[warning] 74-74: 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(.*workflow/${workflowName}.*)
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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Server
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (9)
e2e/pages/workflow-list.page.ts (2)
21-39: LGTM! Well-structured locator initialization.The use of content-based selectors for HeadlessUI components and the exclusion of loading states (line 37) demonstrates good understanding of UI testing best practices.
44-67: LGTM! Robust synchronization strategy.The use of multiple wait states (DOM, visibility, network) combined with
Promise.racefor handling both success and error states is a solid approach for reliable test execution.e2e/tests/workflow-crud.spec.ts (2)
16-58: LGTM! Well-structured tests with proper cleanup.The tests demonstrate good practices:
- Clear test objectives
- Proper use of page objects
- API verification of UI actions
- Consistent cleanup in
finallyblocks
82-152: LGTM! Comprehensive test coverage.The remaining tests cover important scenarios:
- State toggling with API verification
- Dialog cancellation
- API-to-UI verification
All tests maintain consistent cleanup patterns and verify both UI state and API responses.
e2e/pages/workflow-editor.page.ts (1)
29-57: LGTM! Comprehensive locator initialization.The constructor properly initializes all necessary locators for interacting with the workflow editor, ReactFlow canvas, and UI components.
e2e/helpers/workflow.ts (4)
8-40: LGTM! Well-defined TypeScript interfaces.The interfaces correctly represent Frappe doctype structures:
enabledasnumber(0 or 1) for Check field- JSON fields (
trigger_config,parameters) asstring- Clear separation of concerns between workflow, nodes, and connections
49-72: LGTM! Well-structured helper function.The function properly handles optional parameters and includes helpful documentation about the
trigger_typerequirement when nodes are provided. The JSON stringification oftrigger_configis handled correctly.
100-151: LGTM! Useful convenience functions for tests.These specialized workflow creators provide good examples of different workflow types:
- Schedule Event with cron configuration
- Schedule with Log action node and connections
- Document Event with doctype and event configuration
The node and connection structures in
createScheduleLogWorkflowserve as helpful templates for test scenarios.
160-182: LGTM! Good utility functions for test management.The functions demonstrate good practices:
generateWorkflowTitleuses timestamp + random for uniquenesscleanupTestWorkflowscorrectly filters by title prefix using LIKE- Dynamic import of
getList(line 173) avoids circular dependenciesThe cautionary comment on line 167 is appropriate given the destructive nature of cleanup functions.
| async clickWorkflow(title: string): Promise<void> { | ||
| await this.page.locator(`table tr:has-text("${title}")`).click(); | ||
| await this.page.waitForURL(/.*workflow\/.*/, { timeout: 30000 }); | ||
| } |
There was a problem hiding this comment.
Escape special characters in dynamic selectors.
The title parameter is interpolated directly into the selector without escaping. If the title contains special characters (quotes, parentheses, backslashes), it could break the selector or match unintended elements.
🔒 Proposed fix to escape the selector
async clickWorkflow(title: string): Promise<void> {
- await this.page.locator(`table tr:has-text("${title}")`).click();
+ // Escape quotes in title to prevent selector injection
+ const escapedTitle = title.replace(/["']/g, '\\$&');
+ await this.page.locator(`table tr:has-text("${escapedTitle}")`).click();
await this.page.waitForURL(/.*workflow\/.*/, { timeout: 30000 });
}Alternatively, use a more robust approach with filter:
async clickWorkflow(title: string): Promise<void> {
- await this.page.locator(`table tr:has-text("${title}")`).click();
+ await this.page.locator('table tr').filter({ hasText: title }).click();
await this.page.waitForURL(/.*workflow\/.*/, { timeout: 30000 });
}📝 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.
| async clickWorkflow(title: string): Promise<void> { | |
| await this.page.locator(`table tr:has-text("${title}")`).click(); | |
| await this.page.waitForURL(/.*workflow\/.*/, { timeout: 30000 }); | |
| } | |
| async clickWorkflow(title: string): Promise<void> { | |
| await this.page.locator('table tr').filter({ hasText: title }).click(); | |
| await this.page.waitForURL(/.*workflow\/.*/, { timeout: 30000 }); | |
| } |
🤖 Prompt for AI Agents
In @e2e/pages/workflow-list.page.ts around lines 113 - 116, The clickWorkflow
method interpolates title directly into a CSS selector which can break for
titles containing quotes, parentheses, backslashes, etc.; fix by either escaping
special characters in title before using it in page.locator(`table
tr:has-text("${title}")`) or switch to a robust locator pattern: first select
the row elements (e.g., table tr) and then filter by visible text using
Playwright's has/hasText/filter APIs (refer to clickWorkflow and the
page.locator usage) so the dynamic title is matched safely without injecting raw
characters into the selector.
| async toggleWorkflowEnabled(title: string): Promise<void> { | ||
| const row = this.page.locator(`table tr:has-text("${title}")`); | ||
| const toggle = row.locator('button[role="switch"]'); | ||
| await toggle.click(); | ||
| // Wait for API response | ||
| await this.page.waitForResponse( | ||
| (resp) => | ||
| resp.url().includes('/api/resource/Hazel%20Workflow') && | ||
| resp.request().method() === 'PUT' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Escape special characters in dynamic selectors.
Same issue as in clickWorkflow: the title parameter is interpolated without escaping.
🔒 Proposed fix to use Playwright's filter method
async toggleWorkflowEnabled(title: string): Promise<void> {
- const row = this.page.locator(`table tr:has-text("${title}")`);
+ const row = this.page.locator('table tr').filter({ hasText: title });
const toggle = row.locator('button[role="switch"]');
await toggle.click();
// Wait for API response
await this.page.waitForResponse(
(resp) =>
resp.url().includes('/api/resource/Hazel%20Workflow') &&
resp.request().method() === 'PUT'
);
}📝 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.
| async toggleWorkflowEnabled(title: string): Promise<void> { | |
| const row = this.page.locator(`table tr:has-text("${title}")`); | |
| const toggle = row.locator('button[role="switch"]'); | |
| await toggle.click(); | |
| // Wait for API response | |
| await this.page.waitForResponse( | |
| (resp) => | |
| resp.url().includes('/api/resource/Hazel%20Workflow') && | |
| resp.request().method() === 'PUT' | |
| ); | |
| } | |
| async toggleWorkflowEnabled(title: string): Promise<void> { | |
| const row = this.page.locator('table tr').filter({ hasText: title }); | |
| const toggle = row.locator('button[role="switch"]'); | |
| await toggle.click(); | |
| // Wait for API response | |
| await this.page.waitForResponse( | |
| (resp) => | |
| resp.url().includes('/api/resource/Hazel%20Workflow') && | |
| resp.request().method() === 'PUT' | |
| ); | |
| } |
🤖 Prompt for AI Agents
In @e2e/pages/workflow-list.page.ts around lines 121 - 131, The dynamic locator
in toggleWorkflowEnabled interpolates title directly into a CSS selector which
breaks for special characters; update toggleWorkflowEnabled to locate the row
using a safe filter (e.g., use this.page.locator('table tr').filter({ hasText:
title }) or Playwright's getByRole/getByText filtering) instead of `table
tr:has-text("${title}")`, then find the switch button via
row.locator('button[role="switch"]') and proceed; mirror the same change you
applied to clickWorkflow so titles with special characters are handled safely.
| async workflowExists(title: string): Promise<boolean> { | ||
| const row = this.page.locator(`table tr:has-text("${title}")`); | ||
| return (await row.count()) > 0; | ||
| } |
There was a problem hiding this comment.
Escape special characters in dynamic selectors.
Same selector injection issue. Use Playwright's filter method for safer element matching.
🔒 Proposed fix
async workflowExists(title: string): Promise<boolean> {
- const row = this.page.locator(`table tr:has-text("${title}")`);
+ const row = this.page.locator('table tr').filter({ hasText: title });
return (await row.count()) > 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async workflowExists(title: string): Promise<boolean> { | |
| const row = this.page.locator(`table tr:has-text("${title}")`); | |
| return (await row.count()) > 0; | |
| } | |
| async workflowExists(title: string): Promise<boolean> { | |
| const row = this.page.locator('table tr').filter({ hasText: title }); | |
| return (await row.count()) > 0; | |
| } |
🤖 Prompt for AI Agents
In @e2e/pages/workflow-list.page.ts around lines 136 - 139, The dynamic selector
in workflowExists uses string interpolation into a selector and can be
exploited; replace the `this.page.locator(\`table tr:has-text("${title}")\`)`
usage with Playwright's safer filter pattern by locating rows first (e.g.,
`this.page.locator('table tr')`) and then applying `.filter({ hasText: title })`
before counting, so the title is treated as data rather than injected into the
selector.
| async expectWorkflowVisible(title: string): Promise<void> { | ||
| await expect(this.page.locator(`table tr:has-text("${title}")`)).toBeVisible(); | ||
| } |
There was a problem hiding this comment.
Escape special characters in dynamic selectors.
Same selector injection issue.
🔒 Proposed fix
async expectWorkflowVisible(title: string): Promise<void> {
- await expect(this.page.locator(`table tr:has-text("${title}")`)).toBeVisible();
+ await expect(this.page.locator('table tr').filter({ hasText: title })).toBeVisible();
}🤖 Prompt for AI Agents
In @e2e/pages/workflow-list.page.ts around lines 152 - 154, The dynamic selector
in expectWorkflowVisible(title) uses string interpolation which can break or be
exploited if title contains special characters; change the locator to use
Playwright's hasText option instead of embedding the title into the selector
(i.e., locate 'table tr' and pass { hasText: title }) so the title is treated as
text content rather than part of the selector; update the expect call to use
that locator and remove the interpolated selector.
Use force:true on click to bypass any intercepting dialog panels that may overlay the controls. This handles cases where HeadlessUI dialogs or other overlays might intercept pointer events.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/pages/workflow-editor.page.ts (2)
113-132: UnusedcanvasBoundsvariable.The
canvasBoundsis retrieved at line 123 but only used for a null check. If the intent was solely to verify canvas existence,await this.canvas.waitFor({ state: 'visible' })would suffice. Otherwise, if bounds-based calculations were planned, they appear incomplete.♻️ Suggested simplification
async dragNodeToCanvas( nodeName: string, targetX: number, targetY: number ): Promise<void> { const paletteNode = this.getPaletteNode(nodeName); // Ensure palette node is visible await paletteNode.waitFor({ state: 'visible', timeout: 10000 }); - const canvasBounds = await this.canvas.boundingBox(); - if (!canvasBounds) { - throw new Error('Canvas not found'); - } + // Ensure canvas is visible before dragging + await this.canvas.waitFor({ state: 'visible' }); // Drag to canvas at specified position await paletteNode.dragTo(this.canvas, { targetPosition: { x: targetX, y: targetY }, }); }
182-192: Minor: Hardcoded timeout for selection state.The
waitForTimeout(100)at line 189 is a test smell, but acceptable here since waiting for an implicit selection state is difficult to observe otherwise. If flakiness occurs, consider waiting for a more deterministic condition (e.g., a state change in ReactFlow's internal API or a specific attribute).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/pages/workflow-editor.page.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: BuildWithHussain/hazelnode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:50:59.725Z
Learning: Run tests using: bench --site test_site run-tests --app hazelnode
🧬 Code graph analysis (1)
e2e/pages/workflow-editor.page.ts (1)
e2e/pages/index.ts (1)
WorkflowEditorPage(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Playwright E2E Tests
- GitHub Check: Server
🔇 Additional comments (6)
e2e/pages/workflow-editor.page.ts (6)
1-57: LGTM - Clean Page Object structure.The locator declarations follow good POM patterns. Some selectors (e.g.,
[class*="scroll-area"], structural selectors likeh4:has-text(...) + div) are somewhat fragile and may break with UI refactors. Consider addingdata-testidattributes to critical elements in the application code over time for more stable selectors.
59-76: LGTM - Robust wait strategy.The multi-stage wait approach (DOM ready → canvas visible → network idle → controls visible) provides good reliability for CI environments with varying performance.
194-205: LGTM - Save with response verification.Waiting for the specific PUT response ensures save completion before proceeding. The URL-encoded resource name matching is correct.
224-254: LGTM - Force-click workaround documented.The use of
{ force: true }to bypass HeadlessUI dialog overlays is a pragmatic workaround as noted in the commit message. Consider filing a separate issue to investigate why overlays intercept these controls, as this may indicate a z-index or pointer-events issue in the UI.
284-296: LGTM - Correct implementation.The method correctly retrieves all node IDs by iterating locators and extracting the
data-idattribute.
78-108: LGTM - Helper methods are well-structured.The getter, selection, connection, and assertion methods follow clean POM patterns. The
getControlButtonmethod's dual-attribute matching (aria-labelandtitle) is a good defensive approach for ReactFlow version compatibility.Also applies to: 134-167, 207-222, 256-282
- Use both 'github' and 'html' reporters in CI for annotations and artifacts - Only upload test-results/ directory on failure (it's empty when tests pass)
NagariaHussain
changed the title
2 participants
Research from frappe/lms Cypress setup adapted for Playwright:
Summary by CodeRabbit
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.