Conversation
Add a new /interactive page with dynamic a11y violations triggered by user events (click, input, submit) to showcase the auto-scan feature. Update navigation layout and document the new page's intentional issues. Also clarify the WCAG filter label in the ControlPanel.
commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request adds a comprehensive accessibility testing utilities module for the project. It introduces build configuration for a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes span multiple new source files with varying complexity levels. The browser.ts file (279 lines) contains intricate Playwright integration including dynamic mutation observer script generation and axe-core injection. The auto-scan utility implements multi-route result aggregation with filtering logic. Type definitions, matchers, and setup files are more straightforward. Multiple files have interdependencies requiring understanding of overall module architecture. Test coverage is extensive, providing validation but requiring separate reasoning for each test scenario. The heterogeneity of concerns (configuration, types, utilities, browser functions, matchers) and density of logic, particularly in browser integration, place this at the upper end of moderate complexity. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
build.config.ts (1)
5-10: Consider collecting warnings to delete before removing them.Modifying a
Setwhile iterating over it can lead to unexpected behavior in some edge cases. Although modern JavaScript engines typically handle this correctly forSet, a safer pattern would be to collect items first, then delete.♻️ Safer iteration pattern
'build:done'(ctx) { - for (const warning of ctx.warnings) { - if (warning.includes('test-utils')) { - ctx.warnings.delete(warning) - } - } + const toRemove = [...ctx.warnings].filter(w => w.includes('test-utils')) + for (const warning of toRemove) { + ctx.warnings.delete(warning) + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.config.ts` around lines 5 - 10, In the 'build:done' handler, avoid mutating ctx.warnings while iterating; instead iterate once to collect matching warnings (e.g., push into a toDelete array when warning.includes('test-utils')), then iterate over that collection and call ctx.warnings.delete(w) for each item; update the 'build:done' function to use this two-step collect-then-delete pattern referencing ctx.warnings and the 'test-utils' predicate.src/test-utils/format.ts (1)
59-63: Use a null-prototype accumulator ingroupByRule.Using
{}as a dictionary can collide with special keys (e.g.__proto__) for custom rule IDs; a null-prototype object is safer.♻️ Proposed tweak
function groupByRule(violations: A11yViolation[]): Record<string, A11yViolation[]> { - const grouped: Record<string, A11yViolation[]> = {} + const grouped: Record<string, A11yViolation[]> = Object.create(null) for (const v of violations) { (grouped[v.id] ??= []).push(v) } return grouped }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test-utils/format.ts` around lines 59 - 63, The accumulator in function groupByRule currently uses a plain object (`{}`) which can collide with prototype keys like "__proto__"; change the initializer for the `grouped` variable to a null-prototype object (e.g., `Object.create(null)`) while keeping the type as Record<string, A11yViolation[]> and leave the rest of the loop logic (`for (const v of violations) { (grouped[v.id] ??= []).push(v) }`) unchanged so rule IDs cannot shadow prototype properties.src/test-utils/auto-scan.ts (1)
1-3: Break theauto-scan↔indeximport cycle.
src/test-utils/auto-scan.tsimportingrunA11yScanfrom./indexwhilesrc/test-utils/index.tsre-exportscreateAutoScanfrom this file creates a circular dependency. Please move shared scan construction into a lower-level module and import from that module in both places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test-utils/auto-scan.ts` around lines 1 - 3, There is a circular import between createAutoScan (from this file) and runA11yScan (imported from ./index); to fix it extract the shared scan construction and any helper functions/types used by both into a new lower-level module (e.g., scan-core or scan-builder) and move the implementation that currently lives in auto-scan that constructs scan options/results into that module; then have both auto-scan (which should export createAutoScan) and index import runA11yScan or the shared helpers from the new module instead of from each other, updating imports to reference the new module and keeping public exports (createAutoScan, runA11yScan, AutoScanOptions, ScanResult) unchanged.test/e2e/test-utils-playwright.test.ts (1)
16-116: Always close Playwright pages infinally.Current close calls are after assertions; a failing assertion can skip
page.close(), causing browser resource leakage across e2e runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-utils-playwright.test.ts` around lines 16 - 116, Each test opens a Playwright page with createPage() but calls page.close() after assertions, risking leaks if assertions throw; update each it block that uses createPage() (including tests calling runAxeOnPage, injectAxe, and any that assert toHaveNoA11yViolations) to wrap the test body in try { ... } finally { if (page) await page.close() } so the page is always closed even on failures; locate uses of page.close and replace them with a finally close block around the existing logic.test/e2e/test-utils-vitest.test.ts (1)
61-68: Test title and assertion intent are inconsistent.The case says "returns empty array" but never asserts emptiness. Either rename the test or add an explicit length check to match the stated behavior.
✏️ Minimal rename option
- it('getByImpact returns empty array for non-matching level', async () => { + it('getByImpact returns only violations matching the requested level', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-utils-vitest.test.ts` around lines 61 - 68, The test named "getByImpact returns empty array for non-matching level" is inconsistent with its assertions—either rename the test to reflect that it verifies all returned items have impact 'minor', or actually assert emptiness; to fix quickly, add an explicit assertion on minor.length (e.g., expect(minor.length).toBe(0)) after obtaining minor from result.getByImpact('minor'), or alternatively change the it(...) description to something like "getByImpact returns only items with matching impact" to match the existing expect(minor.every(...)) check; locate the test using the it(...) string and the calls to runA11yScan and result.getByImpact to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 48-56: Update .gitignore to avoid overly generic patterns: remove
or narrow the entries named "tasks" and "config.yml" (they are the risky generic
ignores referenced in the diff) and either replace them with project-specific
paths/patterns (e.g., tasks/<tool-name> or config.<env>.yml) or move them to
developer-local excludes (.git/info/exclude). Also add a brief comment above the
Beads/Dolt/Ralph-TUI entries (.beads, .dolt, .doltcfg, beads_a11y, .ralph-tui)
explaining why each is ignored or remove them if those tools aren’t used in the
repo.
In `@playground/app/pages/interactive.vue`:
- Line 1: The component file interactive.vue triggers the
vue/multi-word-component-names lint rule; fix by renaming the component to a
multi-word name (e.g., InteractiveDashboard or interactive-dashboard) and
updating the component's default export/name (or the <script setup>
defineComponent name) to match, or if you intentionally want a single-word name,
add an ESLint disable comment at the top of interactive.vue (/* eslint-disable
vue/multi-word-component-names */) and ensure the filename and component name
remain consistent.
- Line 63: The <option value="">Select role</option> line in interactive.vue
violates the project's formatting rules; reformat that option to match the
surrounding select options (use the same indentation, spacing, and line breaks
as the other <option> entries in the file) and then run the repository formatter
(e.g., Prettier/ESLint fix) or save with the editor's formatting to apply the
correct style so the line is consistent with the rest of the template.
- Line 105: Replace the inline style on the price paragraph with a semantic CSS
class: remove style="color: `#999`;" from the <p> that renders {{ p.price }} and
add a class like "muted" (or use your existing utility class, e.g.
text-gray-500) to that <p>; then define the corresponding CSS rule (.muted {
color: `#999`; }) in the component's scoped <style> block (or use the
global/styles utility) so styling is kept out of the markup and the formatting
violation is resolved.
- Line 80: Replace the inline style on the <p> that renders the profile bio (the
element containing {{ profile.bio || 'Bio' }}) with the project's CSS class for
muted text (e.g., class="text-muted") and ensure spacing/indentation matches the
component's template style; keep the same content expression (profile.bio ||
'Bio') but remove the style attribute so formatting/linting rules are satisfied.
- Around line 28-29: The linter flags
vue/singleline-html-element-content-newline for the notification elements;
update the <div class="notification-name"> and the <p style="color: `#bbb`;">
elements to use multiline content style so their text nodes are on separate
lines (e.g. wrap {{ n.name }} and {{ n.message }} on their own lines inside the
respective tags), ensuring consistency for the notification-name element and the
paragraph that renders n.message.
In `@src/test-utils/index.ts`:
- Around line 60-62: The runA11yScan function currently hardcodes the route
string 'test' when calling runAxeOnHtml, which loses real route context; change
runA11yScan to accept a route (e.g., add a route parameter or reuse an existing
options.route) and pass that route into runAxeOnHtml instead of the hardcoded
'test', ensuring createScanResult still receives the violations unchanged;
update callers such as createAutoScan.scanFetchedHtml(url, html) to provide the
actual URL/route when invoking runA11yScan so scans retain correct route
context.
In `@src/test-utils/playwright.ts`:
- Around line 98-107: The current polling loop around w.axe._running in the
page.evaluate callback can still race when multiple callers enter before the
first run flips _running; replace that ad-hoc wait with a serialized in-page
queue: on the page (inside the page.evaluate callback) create/consume a
dedicated run chain or queue variable (e.g., attach w.__axeRunChain or
w.__axeRunQueue) and enqueue each request so runs execute sequentially by
chaining promises before calling w.axe.run(document,...); update the logic
around w.axe._running and w.axe.run in the page.evaluate callback (the block
that assigns rawViolations) to use this in-page queueing mechanism to ensure no
concurrent axe.run executions.
---
Nitpick comments:
In `@build.config.ts`:
- Around line 5-10: In the 'build:done' handler, avoid mutating ctx.warnings
while iterating; instead iterate once to collect matching warnings (e.g., push
into a toDelete array when warning.includes('test-utils')), then iterate over
that collection and call ctx.warnings.delete(w) for each item; update the
'build:done' function to use this two-step collect-then-delete pattern
referencing ctx.warnings and the 'test-utils' predicate.
In `@src/test-utils/auto-scan.ts`:
- Around line 1-3: There is a circular import between createAutoScan (from this
file) and runA11yScan (imported from ./index); to fix it extract the shared scan
construction and any helper functions/types used by both into a new lower-level
module (e.g., scan-core or scan-builder) and move the implementation that
currently lives in auto-scan that constructs scan options/results into that
module; then have both auto-scan (which should export createAutoScan) and index
import runA11yScan or the shared helpers from the new module instead of from
each other, updating imports to reference the new module and keeping public
exports (createAutoScan, runA11yScan, AutoScanOptions, ScanResult) unchanged.
In `@src/test-utils/format.ts`:
- Around line 59-63: The accumulator in function groupByRule currently uses a
plain object (`{}`) which can collide with prototype keys like "__proto__";
change the initializer for the `grouped` variable to a null-prototype object
(e.g., `Object.create(null)`) while keeping the type as Record<string,
A11yViolation[]> and leave the rest of the loop logic (`for (const v of
violations) { (grouped[v.id] ??= []).push(v) }`) unchanged so rule IDs cannot
shadow prototype properties.
In `@test/e2e/test-utils-playwright.test.ts`:
- Around line 16-116: Each test opens a Playwright page with createPage() but
calls page.close() after assertions, risking leaks if assertions throw; update
each it block that uses createPage() (including tests calling runAxeOnPage,
injectAxe, and any that assert toHaveNoA11yViolations) to wrap the test body in
try { ... } finally { if (page) await page.close() } so the page is always
closed even on failures; locate uses of page.close and replace them with a
finally close block around the existing logic.
In `@test/e2e/test-utils-vitest.test.ts`:
- Around line 61-68: The test named "getByImpact returns empty array for
non-matching level" is inconsistent with its assertions—either rename the test
to reflect that it verifies all returned items have impact 'minor', or actually
assert emptiness; to fix quickly, add an explicit assertion on minor.length
(e.g., expect(minor.length).toBe(0)) after obtaining minor from
result.getByImpact('minor'), or alternatively change the it(...) description to
something like "getByImpact returns only items with matching impact" to match
the existing expect(minor.every(...)) check; locate the test using the it(...)
string and the calls to runA11yScan and result.getByImpact to make the change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
.gitignorebuild.config.test-utils.tsbuild.config.tsclient/app/components/ControlPanel.vuepackage.jsonplayground/A11Y-ISSUES.mdplayground/app/layouts/default.vueplayground/app/pages/interactive.vuesrc/test-utils/auto-scan.tssrc/test-utils/format.tssrc/test-utils/index.tssrc/test-utils/matchers.tssrc/test-utils/playwright.tssrc/test-utils/setup.tssrc/test-utils/types.tstest/e2e/test-utils-auto-scan.test.tstest/e2e/test-utils-playwright.test.tstest/e2e/test-utils-vitest.test.tstest/unit/test-utils-auto-scan.test.tstest/unit/test-utils-format.test.tstest/unit/test-utils-matchers.test.tstest/unit/test-utils-scan.test.ts
playground/app/pages/interactive.vue
Outdated
| :src="`https://placehold.co/80x80/764ba2/ffffff?text=${profile.name.charAt(0)}`" | ||
| > | ||
| <h6>{{ profile.name || 'Name' }}</h6> | ||
| <p style="color: #aaa;">{{ profile.bio || 'Bio' }}</p> |
There was a problem hiding this comment.
Fix formatting violation.
🔧 Proposed fix
- <p style="color: `#aaa`;">{{ profile.bio || 'Bio' }}</p>
+ <p style="color: `#aaa`;">
+ {{ profile.bio || 'Bio' }}
+ </p>📝 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.
| <p style="color: #aaa;">{{ profile.bio || 'Bio' }}</p> | |
| <p style="color: `#aaa`;"> | |
| {{ profile.bio || 'Bio' }} | |
| </p> |
🧰 Tools
🪛 ESLint
[error] 80-80: Expected 1 line break after opening tag (<p>), but no line breaks found.
(vue/singleline-html-element-content-newline)
[error] 80-80: Expected 1 line break before closing tag (</p>), but no line breaks found.
(vue/singleline-html-element-content-newline)
🪛 GitHub Check: ci (ubuntu-latest)
[failure] 80-80:
Expected 1 line break before closing tag (</p>), but no line breaks found
[failure] 80-80:
Expected 1 line break after opening tag (<p>), but no line breaks found
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playground/app/pages/interactive.vue` at line 80, Replace the inline style on
the <p> that renders the profile bio (the element containing {{ profile.bio ||
'Bio' }}) with the project's CSS class for muted text (e.g., class="text-muted")
and ensure spacing/indentation matches the component's template style; keep the
same content expression (profile.bio || 'Bio') but remove the style attribute so
formatting/linting rules are satisfied.
playground/app/pages/interactive.vue
Outdated
| > | ||
| <img :src="p.image"> | ||
| <h3>{{ p.name }}</h3> | ||
| <p style="color: #999;">{{ p.price }}</p> |
There was a problem hiding this comment.
Fix formatting violation.
🔧 Proposed fix
- <p style="color: `#999`;">{{ p.price }}</p>
+ <p style="color: `#999`;">
+ {{ p.price }}
+ </p>📝 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.
| <p style="color: #999;">{{ p.price }}</p> | |
| <p style="color: `#999`;"> | |
| {{ p.price }} | |
| </p> |
🧰 Tools
🪛 ESLint
[error] 105-105: Expected 1 line break after opening tag (<p>), but no line breaks found.
(vue/singleline-html-element-content-newline)
[error] 105-105: Expected 1 line break before closing tag (</p>), but no line breaks found.
(vue/singleline-html-element-content-newline)
🪛 GitHub Check: ci (ubuntu-latest)
[failure] 105-105:
Expected 1 line break after opening tag (<p>), but no line breaks found
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playground/app/pages/interactive.vue` at line 105, Replace the inline style
on the price paragraph with a semantic CSS class: remove style="color: `#999`;"
from the <p> that renders {{ p.price }} and add a class like "muted" (or use
your existing utility class, e.g. text-gray-500) to that <p>; then define the
corresponding CSS rule (.muted { color: `#999`; }) in the component's scoped
<style> block (or use the global/styles utility) so styling is kept out of the
markup and the formatting violation is resolved.
src/test-utils/playwright.ts
Outdated
| const rawViolations: RawAxeViolation[] = await page.evaluate(async ({ axeOptions: spec, runOptions: run }) => { | ||
| const w = window as Record<string, any> | ||
|
|
||
| while (w.axe._running) { | ||
| await new Promise(r => setTimeout(r, 100)) | ||
| } | ||
|
|
||
| if (spec) w.axe.configure(spec) | ||
| return w.axe.run(document, run || {}).then((results: any) => results.violations) | ||
| }, { axeOptions: axeOptions || null, runOptions: runOptions || null }) |
There was a problem hiding this comment.
Serialize axe execution per page to avoid racey concurrent scans.
Current waiting logic can still race when two calls enter before one run flips the internal running state. This can make scans flaky under parallel callers.
🔧 Proposed fix (queue runs in page context)
- const rawViolations: RawAxeViolation[] = await page.evaluate(async ({ axeOptions: spec, runOptions: run }) => {
- const w = window as Record<string, any>
-
- while (w.axe._running) {
- await new Promise(r => setTimeout(r, 100))
- }
-
- if (spec) w.axe.configure(spec)
- return w.axe.run(document, run || {}).then((results: any) => results.violations)
- }, { axeOptions: axeOptions || null, runOptions: runOptions || null })
+ const rawViolations: RawAxeViolation[] = await page.evaluate(async ({ axeOptions: spec, runOptions: run }) => {
+ const w = window as Record<string, any>
+ w.__nuxtA11yAxeQueue ??= Promise.resolve()
+
+ const execute = async () => {
+ if (spec) w.axe.configure(spec)
+ const results = await w.axe.run(document, run || {})
+ return results.violations
+ }
+
+ const queued = w.__nuxtA11yAxeQueue.then(execute, execute)
+ w.__nuxtA11yAxeQueue = queued.catch(() => undefined)
+ return queued
+ }, { axeOptions: axeOptions || null, runOptions: runOptions || null })📝 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.
| const rawViolations: RawAxeViolation[] = await page.evaluate(async ({ axeOptions: spec, runOptions: run }) => { | |
| const w = window as Record<string, any> | |
| while (w.axe._running) { | |
| await new Promise(r => setTimeout(r, 100)) | |
| } | |
| if (spec) w.axe.configure(spec) | |
| return w.axe.run(document, run || {}).then((results: any) => results.violations) | |
| }, { axeOptions: axeOptions || null, runOptions: runOptions || null }) | |
| const rawViolations: RawAxeViolation[] = await page.evaluate(async ({ axeOptions: spec, runOptions: run }) => { | |
| const w = window as Record<string, any> | |
| w.__nuxtA11yAxeQueue ??= Promise.resolve() | |
| const execute = async () => { | |
| if (spec) w.axe.configure(spec) | |
| const results = await w.axe.run(document, run || {}) | |
| return results.violations | |
| } | |
| const queued = w.__nuxtA11yAxeQueue.then(execute, execute) | |
| w.__nuxtA11yAxeQueue = queued.catch(() => undefined) | |
| return queued | |
| }, { axeOptions: axeOptions || null, runOptions: runOptions || null }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test-utils/playwright.ts` around lines 98 - 107, The current polling loop
around w.axe._running in the page.evaluate callback can still race when multiple
callers enter before the first run flips _running; replace that ad-hoc wait with
a serialized in-page queue: on the page (inside the page.evaluate callback)
create/consume a dedicated run chain or queue variable (e.g., attach
w.__axeRunChain or w.__axeRunQueue) and enqueue each request so runs execute
sequentially by chaining promises before calling w.axe.run(document,...); update
the logic around w.axe._running and w.axe.run in the page.evaluate callback (the
block that assigns rawViolations) to use this in-page queueing mechanism to
ensure no concurrent axe.run executions.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e/test-utils-vitest.test.ts (1)
14-16: Consider ascanRoute()helper to remove duplication and keep route context in failures.Most tests repeat fetch+scan, and scans currently omit
route. A tiny helper will simplify the suite and make assertion messages route-aware.✨ Suggested refactor
async function fetchHtml(route: string): Promise<string> { return await $fetch<string>(route, { responseType: 'text' }) } + +async function scanRoute(route: string) { + const html = await fetchHtml(route) + return runA11yScan(html, { route }) +}-const html = await fetchHtml('/') -const result = await runA11yScan(html) +const result = await scanRoute('/')Also applies to: 19-20, 29-30, 37-38, 44-46, 62-63, 71-72, 79-80, 88-89, 104-105, 113-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-utils-vitest.test.ts` around lines 14 - 16, Tests duplicate the pattern of calling fetchHtml(route) then running scan(...) without passing route info, which loses route context on failures; add a helper named scanRoute(route: string) that calls fetchHtml(route) and then calls the existing scan(html, route) (or scan(html, { route })) so assertion messages include the route, and replace repeated fetch+scan usages with scanRoute in tests that currently call fetchHtml (function fetchHtml) followed by scan.src/test-utils/auto-scan.ts (1)
2-2: Avoid importing from the root entrypoint insideauto-scan.Line 2 introduces an
index.ts ↔ auto-scan.tscircular dependency (becauseindex.tsre-exportscreateAutoScanfromauto-scan.ts). Import scan primitives from a leaf module to keep module initialization deterministic and avoid issues with bundlers.♻️ Refactor direction
- import { runA11yScan } from './index' + import { runA11yScan } from './scan'// src/test-utils/scan.ts (new leaf module) // move createScanResult + runA11yScan here and import from this file in both index.ts and auto-scan.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test-utils/auto-scan.ts` at line 2, The import in auto-scan.ts of runA11yScan from the package root creates a circular dependency with index.ts (which re-exports createAutoScan from auto-scan.ts); fix by extracting the scan primitives into a new leaf module (e.g., create src/test-utils/scan.ts) that defines and exports runA11yScan and createScanResult, then update both index.ts and auto-scan.ts to import runA11yScan/createScanResult from that new scan.ts instead of from the root entrypoint so module initialization is deterministic and avoids the circular reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test-utils/auto-scan.ts`:
- Line 2: The import in auto-scan.ts of runA11yScan from the package root
creates a circular dependency with index.ts (which re-exports createAutoScan
from auto-scan.ts); fix by extracting the scan primitives into a new leaf module
(e.g., create src/test-utils/scan.ts) that defines and exports runA11yScan and
createScanResult, then update both index.ts and auto-scan.ts to import
runA11yScan/createScanResult from that new scan.ts instead of from the root
entrypoint so module initialization is deterministic and avoids the circular
reference.
In `@test/e2e/test-utils-vitest.test.ts`:
- Around line 14-16: Tests duplicate the pattern of calling fetchHtml(route)
then running scan(...) without passing route info, which loses route context on
failures; add a helper named scanRoute(route: string) that calls
fetchHtml(route) and then calls the existing scan(html, route) (or scan(html, {
route })) so assertion messages include the route, and replace repeated
fetch+scan usages with scanRoute in tests that currently call fetchHtml
(function fetchHtml) followed by scan.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignoresrc/test-utils/auto-scan.tssrc/test-utils/index.tssrc/test-utils/types.tstest/e2e/test-utils-vitest.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- src/test-utils/types.ts
timdamen
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/unit/test-utils-scan.test.ts (3)
93-99: Same vacuous truth concern applies here.If
runOnly: ['image-alt']produces no violations,every()returnstrueon an empty array. Consider addingexpect(result.violationCount).toBeGreaterThan(0)before the assertion.Proposed fix
it('passes ScanOptions through to runAxeOnHtml()', async () => { const result = await runA11yScan(INACCESSIBLE_HTML, { runOptions: { runOnly: { type: 'rule', values: ['image-alt'] } }, }) + expect(result.violationCount).toBeGreaterThan(0) expect(result.violations.every(v => v.id === 'image-alt')).toBe(true) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test-utils-scan.test.ts` around lines 93 - 99, The test using runA11yScan with runOptions currently asserts result.violations.every(...) which can wrongly pass when violations is empty; update the test for the runA11yScan call to first assert result.violationCount (or result.violations.length) is greater than 0 (e.g., expect(result.violationCount).toBeGreaterThan(0)) and then assert that every violation has id 'image-alt' using result.violations to ensure the rule actually produced violations.
101-107: Add a guard assertion to ensure violations exist before checking route.If
result.violationsis empty, the loop body never executes and the test passes vacuously without verifying the route propagation. Adding an assertion ensures the test actually validates the expected behavior.Proposed fix
it('sets route to "test" on violations', async () => { const result = await runA11yScan(INACCESSIBLE_HTML, { route: 'test' }) + expect(result.violationCount).toBeGreaterThan(0) for (const v of result.violations) { expect(v.route).toBe('test') } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test-utils-scan.test.ts` around lines 101 - 107, Add a guard assertion before iterating violations to ensure the test actually validates route propagation: after calling runA11yScan (the result variable) assert that result.violations is non-empty (e.g., expect(result.violations.length).toBeGreaterThan(0) or expect(result.violations).not.toHaveLength(0)) and then keep the existing for loop that checks each v.route === 'test'; this prevents a vacuous pass when result.violations is empty.
60-69: Consider assertingminorreturns an empty array explicitly or document intent.
minor.every(v => v.impact === 'minor')returnstruevacuously when the array is empty. If the intent is to verify correct filtering behavior (even for empty results), consider adding a clarifying comment or asserting the expected length.Proposed clarification
const minor = result.getByImpact('minor') + // Vacuously true if no minor violations exist in fixture; validates filter correctness expect(minor.every(v => v.impact === 'minor')).toBe(true)Or explicitly test the empty case:
const minor = result.getByImpact('minor') - expect(minor.every(v => v.impact === 'minor')).toBe(true) + // INACCESSIBLE_HTML has no minor violations + expect(minor).toEqual([])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test-utils-scan.test.ts` around lines 60 - 69, The test "getByImpact() filters violations by impact level" currently only checks minor.every(...) which vacuously passes for an empty array; update the test that calls runA11yScan(INACCESSIBLE_HTML) and then uses result.getByImpact('minor') to either explicitly assert the expected length (e.g., expect(minor.length).toBe(0) if the intent is an empty result) or add a short clarifying comment above the minor assertion stating that an empty array is the expected outcome; reference the test function name and the result.getByImpact('minor') variable when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/test-utils-scan.test.ts`:
- Around line 93-99: The test using runA11yScan with runOptions currently
asserts result.violations.every(...) which can wrongly pass when violations is
empty; update the test for the runA11yScan call to first assert
result.violationCount (or result.violations.length) is greater than 0 (e.g.,
expect(result.violationCount).toBeGreaterThan(0)) and then assert that every
violation has id 'image-alt' using result.violations to ensure the rule actually
produced violations.
- Around line 101-107: Add a guard assertion before iterating violations to
ensure the test actually validates route propagation: after calling runA11yScan
(the result variable) assert that result.violations is non-empty (e.g.,
expect(result.violations.length).toBeGreaterThan(0) or
expect(result.violations).not.toHaveLength(0)) and then keep the existing for
loop that checks each v.route === 'test'; this prevents a vacuous pass when
result.violations is empty.
- Around line 60-69: The test "getByImpact() filters violations by impact level"
currently only checks minor.every(...) which vacuously passes for an empty
array; update the test that calls runA11yScan(INACCESSIBLE_HTML) and then uses
result.getByImpact('minor') to either explicitly assert the expected length
(e.g., expect(minor.length).toBe(0) if the intent is an empty result) or add a
short clarifying comment above the minor assertion stating that an empty array
is the expected outcome; reference the test function name and the
result.getByImpact('minor') variable when making this change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/test-utils-browser.test.ts (1)
127-157: Replace fixed sleeps with condition-based waits to reduce flakiness.These
waitForTimeout(...)calls make the suite timing-sensitive and slower than necessary. Playwright's official guidance discourages fixed sleeps; prefer condition-based waits instead. For test-side state likeresults.length, useexpect.poll()to automatically retry until the condition is met.♻️ Example pattern
- await page.waitForTimeout(1500) - expect(results.length).toBeGreaterThan(afterClick) + await expect.poll(() => results.length).toBeGreaterThan(afterClick)Also applies to: 173-188, 203-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-utils-browser.test.ts` around lines 127 - 157, Replace the fixed sleeps (page.waitForTimeout calls) with condition-based waits: after actions like await page.click('button.load-btn') wait for the expected DOM/state change (e.g., use expect.poll(() => results.length).toBeGreaterThan(0) or await page.waitForSelector for the element the click injects) instead of sleeping; after filling the input ('input[placeholder="Display name"]') and selecting the option ('select.form-input') use expect.poll(() => results.length).toBeGreaterThan(<previousCount>) or waitForSelector for the preview/updated element; similarly after the page.evaluate DOM injection wait for the new img (await page.waitForSelector('img[src="broken.png"]') or expect.poll(() => results.some(r => r.violationCount > 0))). Update all instances (including the other ranges noted) to remove waitForTimeout and use these condition-based waits so tests retry until the desired state is observed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test-utils/browser.ts`:
- Around line 118-124: The loop that waits on the internal flag w.axe._running
must be hardened with a timeout to avoid indefinite hangs: replace the unbounded
while-await loop with a bounded wait (e.g., poll with a max timeout or attempt
count and abort/error if exceeded) so the caller fails fast instead of hanging;
apply the same pattern to the observer's runScan() retry logic that also checks
w.axe._running. Locate usages referencing w.axe._running and runScan() in this
file and implement a Promise.race or maxAttempts-based polling with a clear
timeout error path so tests won't deadlock if the internal flag never clears.
---
Nitpick comments:
In `@test/e2e/test-utils-browser.test.ts`:
- Around line 127-157: Replace the fixed sleeps (page.waitForTimeout calls) with
condition-based waits: after actions like await page.click('button.load-btn')
wait for the expected DOM/state change (e.g., use expect.poll(() =>
results.length).toBeGreaterThan(0) or await page.waitForSelector for the element
the click injects) instead of sleeping; after filling the input
('input[placeholder="Display name"]') and selecting the option
('select.form-input') use expect.poll(() =>
results.length).toBeGreaterThan(<previousCount>) or waitForSelector for the
preview/updated element; similarly after the page.evaluate DOM injection wait
for the new img (await page.waitForSelector('img[src="broken.png"]') or
expect.poll(() => results.some(r => r.violationCount > 0))). Update all
instances (including the other ranges noted) to remove waitForTimeout and use
these condition-based waits so tests retry until the desired state is observed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build.config.test-utils.tspackage.jsonsrc/test-utils/browser.tssrc/test-utils/index.tssrc/test-utils/types.tstest/e2e/test-utils-browser.test.tstest/unit/test-utils-scan.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- build.config.test-utils.ts
- src/test-utils/index.ts
| while (w.axe._running) { | ||
| await new Promise(r => setTimeout(r, 100)) | ||
| } | ||
|
|
||
| if (spec) w.axe.configure(spec) | ||
| return w.axe.run(document, run || {}).then((results: any) => results.violations) | ||
| }, { axeOptions: axeOptions || null, runOptions: runOptions || null }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the full file to understand the context
wc -l src/test-utils/browser.tsRepository: nuxt/a11y
Length of output: 82
🏁 Script executed:
# Read the relevant sections of the file
cat -n src/test-utils/browser.ts | sed -n '110,135p'Repository: nuxt/a11y
Length of output: 1178
🏁 Script executed:
# Also check lines 223-225 as mentioned
cat -n src/test-utils/browser.ts | sed -n '215,235p'Repository: nuxt/a11y
Length of output: 912
🏁 Script executed:
# Check what version of axe-core is being used
cat package.json | grep -A 5 -B 5 "axe-core"Repository: nuxt/a11y
Length of output: 303
🌐 Web query:
axe-core 4.11.x API reference _running internal field documentation
💡 Result:
In axe-core 4.11.x, axe._running is not a public API—it’s an internal boolean used to prevent concurrent axe.run() executions.
What _running does (4.11.0 example)
In lib/core/public/run.js, axe.run():
- asserts that
axe._runningis falsy, otherwise throws: “Axe is already running…” - sets
axe._running = trueat the start of a run - sets
axe._running = falseon completion and on error paths (before teardown / resolve / reject) [2]
Where to find “official” API docs
Deque’s docs note that the full developer API reference is generated from the repo via JSDoc (npm run api-docs, output under doc/api)—but internal fields like _running typically won’t be treated as stable/documented API [1].
Sources: Deque axe-core API documentation page [1]; axe-core v4.11.0 run.js source showing _running usage [2].
🌐 Web query:
axe-core concurrent axe.run() calls serialization recommended approach
💡 Result:
axe.run() is not re-entrant in a given page context: axe-core keeps a global “in progress” flag (axe._running) and, if you start a second run before the first finishes, it bails with the error “Axe is already running. Use await axe.run() to wait for the previous run to finish…” (i.e., concurrent runs must be serialized) [1][2].
Recommended approach (serialize runs):
- Ensure every call is awaited/returned all the way up the stack (no “fire-and-forget” wrappers), so a later call can’t start until the prior Promise resolves [2].
- If you have multiple triggers (e.g., DOM mutations, multiple components/tests hitting the same page), put
axe.run()behind a single-flight queue/mutex (a simple FIFO Promise chain) and optionally throttle/debounce scans to avoid repeated overlapping work [3].
If you need parallelism:
- Don’t parallelize multiple
axe.run()calls within the samewindow/page. Instead, parallelize by running scans in separate browser pages/contexts/processes, so each has its own axe global state (your test runner can still run workers in parallel; just avoid sharing the same page instance).
Sources: [1] shows the internal _running guard and error message in axe-core’s run implementation; [2] is a real-world report of the error when Cypress tests occasionally overlap runs; [3] is an example of using a task queue/throttling around axe.run() to prevent overlapping scans.
[1] https://gist.github.com/benbcai/43b293128e4e732096cfcf7208555f22
[2] https://stackoverflow.com/questions/79530199/testing-storybook-with-cypress-gets-axe-is-already-running-error
[3] https://accented.dev/how-it-works
Unbounded wait on internal axe._running can deadlock scans.
Line 118 loops until w.axe._running clears without a timeout. _running is an internal, undocumented axe-core field (not part of the public API), and if the flag is stuck, tests hang indefinitely.
🔧 Proposed hardening
- while (w.axe._running) {
- await new Promise(r => setTimeout(r, 100))
- }
+ const startedAt = Date.now()
+ const MAX_WAIT_MS = 10_000
+ while (w.axe?._running) {
+ if (Date.now() - startedAt > MAX_WAIT_MS) {
+ throw new Error('Timed out waiting for in-progress axe scan to finish')
+ }
+ await new Promise(r => setTimeout(r, 100))
+ }Note: Lines 223–225 have a similar check within the observer's runScan() retry logic; apply the same timeout pattern there.
📝 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.
| while (w.axe._running) { | |
| await new Promise(r => setTimeout(r, 100)) | |
| } | |
| if (spec) w.axe.configure(spec) | |
| return w.axe.run(document, run || {}).then((results: any) => results.violations) | |
| }, { axeOptions: axeOptions || null, runOptions: runOptions || null }) | |
| const startedAt = Date.now() | |
| const MAX_WAIT_MS = 10_000 | |
| while (w.axe?._running) { | |
| if (Date.now() - startedAt > MAX_WAIT_MS) { | |
| throw new Error('Timed out waiting for in-progress axe scan to finish') | |
| } | |
| await new Promise(r => setTimeout(r, 100)) | |
| } | |
| if (spec) w.axe.configure(spec) | |
| return w.axe.run(document, run || {}).then((results: any) => results.violations) | |
| }, { axeOptions: axeOptions || null, runOptions: runOptions || null }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test-utils/browser.ts` around lines 118 - 124, The loop that waits on the
internal flag w.axe._running must be hardened with a timeout to avoid indefinite
hangs: replace the unbounded while-await loop with a bounded wait (e.g., poll
with a max timeout or attempt count and abort/error if exceeded) so the caller
fails fast instead of hanging; apply the same pattern to the observer's
runScan() retry logic that also checks w.axe._running. Locate usages referencing
w.axe._running and runScan() in this file and implement a Promise.race or
maxAttempts-based polling with a clear timeout error path so tests won't
deadlock if the internal flag never clears.
1 participant
🔗 Linked issue
#207
📚 Description
Current Step 1 — @nuxt/a11y/test-utils exports runA11yScan(), toHaveNoA11yViolations(), Playwright helpers (injectAxe, runAxeOnPage), and createAutoScan(). Users can already compose these manually.
Step 2 (nuxt/test-utils#1603) makes @nuxt/test-utils an a11y engine: setup({ a11y: true }) and every $fetch and createPage/goto is automatically scanned. Violations aggregate silently, and afterAll reports the summary.