Conversation
commit: |
onmax
force-pushed
the
feat/report-generation
branch
from
6a7e4e7 to
42d157b
Compare
|
Hey @onmax, great work I noticed that your PR also addresses the requirements from two other open issues: #208 - Check accessibility when prerendering pagesThis issue requests exactly what your PR implements:
#207 - Integrate with nuxt/test-utils for testingYour import { runAxeOnHtml } from '@nuxt/a11y/server'
const violations = await runAxeOnHtml(html, '/test-route', {})
expect(violations).toHaveLength(0)Could you link #208 in your PR description? Something like: This way, both issues get closed automatically when the PR merges. For #207, it's not fully addressed, but your PR provides the core utilities needed - that can remain open as a follow-up for adding Vitest matchers and test-utils integration. Do you have time to read #207 and align your code so that it could serve foundation for 207? Thanks! |
|
done! Also happy to help with #207 😃 |
PR Review: Testing ResultsThanks for the PR! I tested this locally and found a couple of issues. 1. Critical: ESM Import Error with
|
|
Thanks for the review! I could reproduce the double-nested path issue and will fix it. However, I could not reproduce the ESM error with
All combinations work without ESM errors. Repro: GitHub · StackBlitz
Could you share:
Output running in the playground
My output running generate in the onmax/repros/a11y-210
|
|
After the clean install, all routes prerendered successfully without the entities ESM error. (mb!) Only the double-nested path issue needs fixing. |
onmax
force-pushed
the
feat/report-generation
branch
2 times, most recently
from
059355a to
3e32ec6
Compare
onmax
force-pushed
the
feat/report-generation
branch
from
3e32ec6 to
7a6df04
Compare
|
i fixed the double nested path issue and make sure pr is ready to be merged please let me know if we need something else :) |
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
this will be great! a few suggested changes. I think we can improve the DX here.
also, I got the same entities issue, which also went away when purging node_modules, dist, etc.
| if (collectedViolations.length > 0) { | ||
| const stats = getStats(collectedViolations) | ||
| logger.warn(`Found ${stats.totalViolations} violations (${stats.byImpact.critical || 0} critical, ${stats.byImpact.serious || 0} serious)`) | ||
| if (reportConfig?.failOnViolation) process.exitCode = 1 |
There was a problem hiding this comment.
I think we could do this from module.ts - and maybe even print out a nice little summary and point people to the full details in the markdown file. (we could pass information back and forth with useStorage)
|
|
||
| try { | ||
| if (options.axeOptions) | ||
| axe.configure(options.axeOptions) |
There was a problem hiding this comment.
it'll be configured lots of times - should we do this only once?
| reportWritten = true | ||
|
|
||
| // Ensure output path is within .nuxt/ to prevent path traversal | ||
| const baseDir = resolve(process.cwd(), '.nuxt') |
There was a problem hiding this comment.
we should not hard code this - it's possible to set a custom base dir
|
|
||
| try { | ||
| await mkdir(dirname(output), { recursive: true }) | ||
| await writeFile(output, report, 'utf-8') |
There was a problem hiding this comment.
ideally we should use nitro storage rather than using node fs directly here
…it's set when either client scanning or report generation is enabled
📝 WalkthroughWalkthroughAdds build-time accessibility report generation to the Nuxt a11y module. Introduces a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 1
🤖 Fix all issues with AI agents
In `@src/module.ts`:
- Around line 88-99: The code resolves the report path with
resolve(nuxt.options.buildDir, options.report.output) but does not validate path
traversal, contradicting the README; update the logic that builds and writes the
report (around formatMarkdownReport, resolve(...), mkdir, writeFile) to verify
the resolved output path is inside nuxt.options.buildDir (e.g., compare path
prefixes or use path.relative and ensure it doesn't start with '..' and is not
absolute outside the buildDir); if validation fails, log a clear error via
logger.error and abort writing instead of proceeding to mkdir/writeFile. Ensure
you reference options.report.output and perform the check before calling
mkdir(dirname(output)) or writeFile.
🧹 Nitpick comments (2)
src/runtime/server/utils/axe-server.ts (1)
79-82: Configuration is only applied whenaxeOptionsis provided on the first call.The current logic skips configuration if
axeOptionsis falsy. If the first call torunAxeOnHtmlhas noaxeOptionsbut subsequent calls do, the configuration will never be applied. This is fine for the current use case where options are consistently passed, but worth noting for future extensibility.♻️ Alternative: Configure once on first non-empty options
- if (options.axeOptions && !configured) { - axe.configure(options.axeOptions) - configured = true - } + if (!configured && options.axeOptions) { + axe.configure(options.axeOptions) + configured = true + }The current implementation is functionally equivalent; this is just a readability suggestion to emphasize the guard first.
src/runtime/server/plugins/a11y-report.ts (1)
12-16: HTML reconstruction from render context.The
constructHtmlhelper correctly assembles the full HTML document from Nuxt's render context parts. One minor note: ifhtmlAttrsorbodyAttrsarrays are empty, this produces<html >with a trailing space, which is harmless but slightly untidy.♻️ Optional: Trim empty attribute strings
function constructHtml(ctx: NuxtRenderHTMLContext): string { - const htmlAttrs = ctx.htmlAttrs.join(' ') - const bodyAttrs = ctx.bodyAttrs.join(' ') - return `<!DOCTYPE html><html ${htmlAttrs}><head>${ctx.head.join('')}</head><body ${bodyAttrs}>${ctx.bodyPrepend.join('')}${ctx.body.join('')}${ctx.bodyAppend.join('')}</body></html>` + const htmlAttrs = ctx.htmlAttrs.length ? ` ${ctx.htmlAttrs.join(' ')}` : '' + const bodyAttrs = ctx.bodyAttrs.length ? ` ${ctx.bodyAttrs.join(' ')}` : '' + return `<!DOCTYPE html><html${htmlAttrs}><head>${ctx.head.join('')}</head><body${bodyAttrs}>${ctx.bodyPrepend.join('')}${ctx.body.join('')}${ctx.bodyAppend.join('')}</body></html>` }
|
|
||
| const report = formatMarkdownReport(violations) | ||
| const output = resolve(nuxt.options.buildDir, options.report.output) | ||
|
|
||
| try { | ||
| await mkdir(dirname(output), { recursive: true }) | ||
| await writeFile(output, report, 'utf-8') | ||
| } | ||
| catch (err) { | ||
| logger.error('Failed to write report:', err) | ||
| return | ||
| } |
There was a problem hiding this comment.
Documentation states path escaping will be ignored, but no validation is implemented.
The README states "Paths that attempt to escape the .nuxt/ directory will be ignored", but the current implementation uses resolve(nuxt.options.buildDir, options.report.output) without path traversal validation. A user could potentially write to locations outside buildDir with ../ in the output path.
🔒 Proposed fix: Validate output path stays within buildDir
const report = formatMarkdownReport(violations)
const output = resolve(nuxt.options.buildDir, options.report.output)
+ // Ensure output stays within buildDir
+ if (!output.startsWith(nuxt.options.buildDir)) {
+ logger.warn(`Report output path escapes build directory, using default location`)
+ output = resolve(nuxt.options.buildDir, 'a11y-report.md')
+ }
+
try {
await mkdir(dirname(output), { recursive: true })Note: Alternatively, update the README to remove the claim about path escaping being ignored if this validation is intentionally omitted.
📝 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 report = formatMarkdownReport(violations) | |
| const output = resolve(nuxt.options.buildDir, options.report.output) | |
| try { | |
| await mkdir(dirname(output), { recursive: true }) | |
| await writeFile(output, report, 'utf-8') | |
| } | |
| catch (err) { | |
| logger.error('Failed to write report:', err) | |
| return | |
| } | |
| const report = formatMarkdownReport(violations) | |
| const output = resolve(nuxt.options.buildDir, options.report.output) | |
| // Ensure output stays within buildDir | |
| if (!output.startsWith(nuxt.options.buildDir)) { | |
| logger.warn(`Report output path escapes build directory, using default location`) | |
| output = resolve(nuxt.options.buildDir, 'a11y-report.md') | |
| } | |
| try { | |
| await mkdir(dirname(output), { recursive: true }) | |
| await writeFile(output, report, 'utf-8') | |
| } | |
| catch (err) { | |
| logger.error('Failed to write report:', err) | |
| return | |
| } |
🤖 Prompt for AI Agents
In `@src/module.ts` around lines 88 - 99, The code resolves the report path with
resolve(nuxt.options.buildDir, options.report.output) but does not validate path
traversal, contradicting the README; update the logic that builds and writes the
report (around formatMarkdownReport, resolve(...), mkdir, writeFile) to verify
the resolved output path is inside nuxt.options.buildDir (e.g., compare path
prefixes or use path.relative and ensure it doesn't start with '..' and is not
absolute outside the buildDir); if validation fails, log a clear error via
logger.error and abort writing instead of proceeding to mkdir/writeFile. Ensure
you reference options.report.output and perform the check before calling
mkdir(dirname(output)) or writeFile.
timdamen
force-pushed
the
feat/report-generation
branch
from
109e316 to
d91bcca
Compare
|
Looks good to me now but I'm no Nitro expert. What do you think @danielroe ? |
3 participants
Closes #208
Closes #209
Summary
Adds build-time accessibility report generation during
nuxt generate/prerender. Runs axe-core server-side via jsdom on each route and generates a markdown report.Features
report.enableddefaults to!nuxt.options.devfailOnViolation: true).nuxt/a11y-report.mdConfiguration
StackBlitz
.nuxt/a11y-report.mdOr locally
Example Output