Add eslint-plugin-playwright enforcement with CI check

[harshach]

Summary

• Add eslint-plugin-playwright to automatically catch common Playwright anti-patterns that cause flaky tests
• Configure 13 rules in two tiers: 3 blocking (error) + 10 aspirational (warn)
• Repurpose the empty ui-checkstyle.yml workflow to run Playwright lint on PRs touching playwright/ files
• Update Playwright Developer Handbook and all 3 Playwright skills with ESLint enforcement docs

Rule Configuration

Blocking (error) — prevents introducing the worst anti-patterns:

Rule	What It Catches
no-networkidle	waitForLoadState('networkidle') — unreliable with websockets/polling
no-page-pause	page.pause() — debug statement left in code
no-focused-test	test.only() / describe.only() — accidentally committed focus

Aspirational (warn) — tracks tech debt to fix over time:
missing-playwright-await, no-wait-for-timeout, no-force-option, no-element-handle, no-eval, no-skipped-test, prefer-web-first-assertions, no-useless-await, no-wait-for-selector, valid-expect

Current state: 0 errors, 1657 warnings (CI passes cleanly)

Cleanup

• Fixed last networkidle usage in ClassificationConditionalRendering.spec.ts
• Removed 3 stale eslint-disable comments referencing undefined rules (jest/no-export, react-hooks/rules-of-hooks)

Test plan

• yarn lint:playwright exits with 0 errors
• Existing yarn lint (for src/) is unaffected
• CI workflow triggers on PRs touching playwright/ files
• Adding networkidle, page.pause(), or test.only() to any Playwright file fails the check

🤖 Generated with Claude Code

[github-actions]

Jest test Coverage

UI tests summary

Lines	Statements	Branches	Functions
	66.04% (57439/86967)	45.65% (30397/66579)	48.63% (9107/18726)

[github-actions]

🟡 Playwright Results — all passed (14 flaky)

✅ 3330 passed · ❌ 0 failed · 🟡 14 flaky · ⏭️ 183 skipped

Shard	Passed	Failed	Flaky	Skipped
🟡 Shard 1	454	0	1	2
🟡 Shard 2	304	0	1	1
🟡 Shard 3	653	0	5	33
🟡 Shard 4	727	0	3	47
✅ Shard 5	591	0	0	67
🟡 Shard 6	601	0	4	33

🟡 14 flaky test(s) (passed on retry)

• Features/DataAssetRulesDisabled.spec.ts › should allow multiple domain selection for glossary term when entity rules are disabled (shard 1, 1 retry)
• Pages/DataContracts.spec.ts › Create Data Contract and validate for Container (shard 2, 1 retry)
• Features/DataQuality/TestCaseIncidentPermissions.spec.ts › User with TEST_CASE.EDIT_ALL can see edit icon on incidents (shard 3, 1 retry)
• Features/DataQuality/TestCaseResultPermissions.spec.ts › User with only VIEW cannot PATCH results (shard 3, 1 retry)
• Features/EntitySummaryPanel.spec.ts › should display summary panel for table (shard 3, 1 retry)
• Features/EntitySummaryPanel.spec.ts › should display summary panel for tableColumn (shard 3, 1 retry)
• Features/Permissions/GlossaryPermissions.spec.ts › Team-based permissions work correctly (shard 3, 1 retry)
• Flow/ObservabilityAlerts.spec.ts › Alert operations for a user with and without permissions (shard 4, 1 retry)
• Pages/Customproperties-part2.spec.ts › entityReferenceList shows item count, scrollable list, no expand toggle (shard 4, 1 retry)
• Pages/DomainUIInteractions.spec.ts › Clear domain selection returns to All Domains (shard 4, 1 retry)
• Pages/ODCSImportExport.spec.ts › Multi-object ODCS contract - object selector shows all schema objects (shard 6, 1 retry)
• Pages/ServiceEntity.spec.ts › Domain Add, Update and Remove (shard 6, 1 retry)
• Pages/ServiceEntity.spec.ts › Inactive Announcement create & delete (shard 6, 1 retry)
• Pages/Users.spec.ts › Permissions for table details page for Data Consumer (shard 6, 1 retry)

📦 Download artifacts

How to debug locally

# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

[karanh37]

@copilot address the security issues in this file .github/workflows/ui-checkstyle.yml

[Copilot]

@karanh37 I've opened a new pull request, #26515, to work on those changes. Once the pull request is ready, I'll request review from you.

[gitar-bot]

CI Analysis

The previous CI failure was caused by commit 5440414 ("Remove unnecessary ANTLR install") which removed the sudo make install_antlr_cli step from the playwright-lint job. The yarn install postinstall script triggers build-check → js-antlr, which requires the antlr4 CLI tool. Without it, the command fails with exit code 127.

This was subsequently fixed in commit 35b4bc74 ("Restore ANTLR CLI install"), and the current HEAD of this branch already includes the Install Antlr4 CLI step before yarn install.

Current status: The ANTLR fix is already in place — no further action needed for this failure.

[gitar-bot]

Code Review ⚠️ Changes requested 6 resolved / 8 findings

Adds eslint-plugin-playwright enforcement with CI checks, addressing critical security issues and workflow configuration problems from the prior attempt. However, the valid-expect rule remains at warn level and will miss missing-await bugs, and four test locations use hardcoded waitForTimeout calls with eslint-disable comments instead of proper element-based waits, undermining the lint goal.

⚠️ Quality: waitForTimeout replaces element waits — opposite of lint goal

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryStatusFilterLargeDataset.spec.ts:159 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryStatusFilterLargeDataset.spec.ts:299 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryStatusFilterLargeDataset.spec.ts:375 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryStatusFilterLargeDataset.spec.ts:508

Four locations replace element-based waits with hardcoded waitForTimeout calls plus eslint-disable comments. This is counterproductive: the PR introduces no-wait-for-timeout as a lint rule, yet these changes add new instances of the exact anti-pattern and suppress the warning. The original code (waiting for table rows to be visible) was semantically closer to correct — it just needed a better Playwright idiom.

Each case has a deterministic, element-based alternative:

1. Line 160 (scroll helper): Use await expect(page.locator('tbody.ant-table-tbody > tr:not([aria-hidden="true"])')).not.toHaveCount(0) — retries until rows appear.
2. Line 299 (after save): Wrap the Save click with waitForResponse matching the glossary terms API (same pattern already used in applyStatusFilter at lines 99-113), then await loader-detach.
3. Line 375 (after clearing search): Use await expect(page.locator('tbody.ant-table-tbody > tr:not([aria-hidden="true"])')).not.toHaveCount(0, { timeout: 10000 }) to poll until rows reappear.
4. Line 508 (after cancel): Use await expect(page.locator('.status-selection-dropdown')).not.toBeVisible() — waits for the dropdown to actually dismiss rather than guessing a 500ms animation duration.

Suggested fix

// Line 160: replace waitForTimeout with row assertion
await expect(
  page.locator('tbody.ant-table-tbody > tr:not([aria-hidden="true"])')
).not.toHaveCount(0, { timeout: 10000 });

// Line 508: replace waitForTimeout with dropdown dismiss
await expect(
  page.locator('.status-selection-dropdown')
).not.toBeVisible();

💡 Quality: valid-expect at warn level won't catch missing-await bugs

📄 openmetadata-ui/src/main/resources/ui/eslint.config.mjs:258 📄 openmetadata-ui/src/main/resources/ui/eslint.config.mjs:257 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataInsight.spec.ts:280

The playwright/valid-expect rule is configured at 'warn' level, but violations of this rule (e.g., non-awaited expect()) represent actual bugs where assertions silently pass without checking anything. The DataInsight.spec.ts fix in this very PR (line 280: adding await to expect(...).toBeVisible()) demonstrates this is a real bug class. Consider promoting valid-expect and missing-playwright-await to error level since both catch silently-passing assertions that give false test confidence.

✅ 6 resolved

✅ Security: pull_request_target + checkout of untrusted code = pwn-request risk

📄 .github/workflows/ui-checkstyle.yml:15 📄 .github/workflows/ui-checkstyle.yml:54 📄 .github/workflows/ui-checkstyle.yml:76 📄 .github/workflows/ui-checkstyle.yml:80 📄 .github/workflows/ui-checkstyle.yml:84
The workflow uses pull_request_target (which runs with write permissions and access to secrets) and then checks out the PR author's code at github.event.pull_request.head.sha. It subsequently executes attacker-controlled code paths:

1. sudo make install_antlr_cli — runs a Makefile from the PR
2. yarn install — executes preinstall/postinstall lifecycle scripts from the PR's package.json
3. yarn lint:playwright — loads eslint.config.mjs and ESLint plugins, all from the PR

The "safe to test" label gate mitigates this (a maintainer must apply the label), but it's a human-only barrier — any synchronize event (new push) re-triggers the workflow without re-verifying the diff, and the guard actions are pinned to mutable version tags (@v1.3.4, @v1.4.0) rather than commit SHAs.

This is consistent with the existing py-checkstyle.yml and java-checkstyle.yml patterns in this repo, so it may be an accepted risk. However, for this workflow specifically, since it only needs read access and no secrets beyond GITHUB_TOKEN, consider using pull_request instead of pull_request_target — the lint check doesn't need elevated permissions.

✅ Performance: CI workflow installs ANTLR CLI unnecessarily for ESLint job

📄 .github/workflows/ui-checkstyle.yml:55
The Install Antlr4 CLI step runs sudo make install_antlr_cli, which downloads a JAR from https://www.antlr.org at runtime. This is completely unused by yarn lint:playwright (a pure ESLint invocation) and adds 10-30s of latency plus an external network dependency that could cause spurious CI failures. This step should be removed from the playwright-lint job.

✅ Quality: CI workflow missing --frozen-lockfile on yarn install

📄 .github/workflows/ui-checkstyle.yml:59
The yarn install step in the playwright-lint job doesn't use --frozen-lockfile, which means it could silently update yarn.lock during CI. While this won't affect the lint result, it's a best practice to use --frozen-lockfile in CI to ensure reproducible installs and catch accidental lockfile drift. Other workflows in this repo follow this pattern.

✅ Bug: Trivially-true assertion in special character search test

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryP3Tests.spec.ts:219
In GlossaryP3Tests.spec.ts, the test "should handle special characters in search" has an assertion that can never fail. The isStable variable is computed as:

(await table.isVisible().catch(() => false)) ||
(await emptyState.isVisible().catch(() => false)) ||
(await tableRows.count()) >= 0

Since tableRows.count() always returns a non-negative number (0 when no rows exist), the expression count >= 0 is always true, making expect(isStable).toBeTruthy() pass unconditionally. Combined with the .catch(() => {}) on line 212 that silently swallows timeout errors, this test would pass even if the page crashed after the search.

The test's intent is to verify the page doesn't crash on special character input, but the assertion doesn't actually validate anything.

✅ Quality: Loader waits use .first().waitFor() instead of waitForAllLoadersToDisappear

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/entity.ts:82 📄 openmetadata-ui/src/main/resources/ui/playwright/utils/entity.ts:109 📄 openmetadata-ui/src/main/resources/ui/playwright/utils/entity.ts:374 📄 openmetadata-ui/src/main/resources/ui/playwright/utils/user.ts:346 📄 openmetadata-ui/src/main/resources/ui/playwright/utils/entity.ts:17
The PR replaces ~29 instances of page.waitForSelector('[data-testid="loader"]', { state: 'detached' }) with page.getByTestId('loader').first().waitFor({ state: 'detached' }). While the old waitForSelector (non-strict mode) also only tracked the first match, the codebase already has a purpose-built helper waitForAllLoadersToDisappear in entity.ts that correctly uses expect(loaders).toHaveCount(0, { timeout }) to wait for ALL loaders to disappear. Since the goal of this PR is to enforce best practices, these replacements should use the existing robust helper rather than the weaker .first().waitFor() pattern. The helper is only used in 3 places currently despite being the recommended approach in the handbook.

...and 1 more resolved from earlier reviews

🤖 Prompt for agents

Code Review: Adds eslint-plugin-playwright enforcement with CI checks, addressing critical security issues and workflow configuration problems from the prior attempt. However, the valid-expect rule remains at warn level and will miss missing-await bugs, and four test locations use hardcoded waitForTimeout calls with eslint-disable comments instead of proper element-based waits, undermining the lint goal.

1. 💡 Quality: valid-expect at warn level won't catch missing-await bugs
   Files: openmetadata-ui/src/main/resources/ui/eslint.config.mjs:258, openmetadata-ui/src/main/resources/ui/eslint.config.mjs:257, openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataInsight.spec.ts:280

   The `playwright/valid-expect` rule is configured at 'warn' level, but violations of this rule (e.g., non-awaited `expect()`) represent actual bugs where assertions silently pass without checking anything. The DataInsight.spec.ts fix in this very PR (line 280: adding `await` to `expect(...).toBeVisible()`) demonstrates this is a real bug class. Consider promoting `valid-expect` and `missing-playwright-await` to error level since both catch silently-passing assertions that give false test confidence.

2. ⚠️ Quality: waitForTimeout replaces element waits — opposite of lint goal
   Files: openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryStatusFilterLargeDataset.spec.ts:159, openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryStatusFilterLargeDataset.spec.ts:299, openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryStatusFilterLargeDataset.spec.ts:375, openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryStatusFilterLargeDataset.spec.ts:508

   Four locations replace element-based waits with hardcoded `waitForTimeout` calls plus `eslint-disable` comments. This is counterproductive: the PR introduces `no-wait-for-timeout` as a lint rule, yet these changes add new instances of the exact anti-pattern and suppress the warning. The original code (waiting for table rows to be visible) was semantically closer to correct — it just needed a better Playwright idiom.
   
   Each case has a deterministic, element-based alternative:
   
   1. **Line 160** (scroll helper): Use `await expect(page.locator('tbody.ant-table-tbody > tr:not([aria-hidden="true"])')).not.toHaveCount(0)` — retries until rows appear.
   2. **Line 299** (after save): Wrap the Save click with `waitForResponse` matching the glossary terms API (same pattern already used in `applyStatusFilter` at lines 99-113), then await loader-detach.
   3. **Line 375** (after clearing search): Use `await expect(page.locator('tbody.ant-table-tbody > tr:not([aria-hidden="true"])')).not.toHaveCount(0, { timeout: 10000 })` to poll until rows reappear.
   4. **Line 508** (after cancel): Use `await expect(page.locator('.status-selection-dropdown')).not.toBeVisible()` — waits for the dropdown to actually dismiss rather than guessing a 500ms animation duration.

   Suggested fix:
   // Line 160: replace waitForTimeout with row assertion
   await expect(
     page.locator('tbody.ant-table-tbody > tr:not([aria-hidden="true"])')
   ).not.toHaveCount(0, { timeout: 10000 });
   
   // Line 508: replace waitForTimeout with dropdown dismiss
   await expect(
     page.locator('.status-selection-dropdown')
   ).not.toBeVisible();

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[sonarqubecloud]

Quality Gate passed for 'open-metadata-ui'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud