html reports#9
Open
katodea wants to merge 3 commits into
Open
Conversation
WalkthroughThis PR converts the QA report generation pipeline from Markdown to self-contained HTML output. The changes add an optional Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/merge-qa-reports.js (1)
107-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
screenshotswhen merging issues.Merged issues currently drop screenshot evidence, which breaks the new HTML screenshot rendering path for merged reports.
Suggested patch
merged.push({ category: issue.category, issue: issue.issue, impact: issue.impact, device: issue.device || 'both', pages: issue.pages || [], + ...(Array.isArray(issue.screenshots) && issue.screenshots.length > 0 && { screenshots: issue.screenshots }), // Include optional fields if present ...(issue.metric && { metric: issue.metric }), ...(issue.wcag_criterion && { wcag_criterion: issue.wcag_criterion }) });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/merge-qa-reports.js` around lines 107 - 116, The merge currently drops issue.screenshots causing missing evidence in merged reports; update the object pushed into merged (the block that builds the merged entry in scripts/merge-qa-reports.js) to include screenshots when present (e.g., add a conditional spread for issue.screenshots similar to metric and wcag_criterion) so merged entries preserve the screenshots array for downstream HTML rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/generate-report.js`:
- Line 93: Update the anchor tags that open in a new tab to include
rel="noopener noreferrer": locate the template that returns the figure HTML (the
expression using safePath, escAttr(filename), and escHtml(filename) which
currently contains <a href="${safePath}" target="_blank">) and add rel="noopener
noreferrer" to that <a> element; do the same for the other similar templates
that render links (the other occurrences that use target="_blank" around
safePath/filename) so all external new-tab links include rel="noopener
noreferrer".
- Around line 81-84: Initialize parts as an empty array instead of seeding it
with 'functional' and only add 'functional' when neither hasPerformanceData nor
hasAccessibilityData is true; specifically, replace the current parts
initialization and pushes so that parts = []; if (hasPerformanceData)
parts.push('performance'); if (hasAccessibilityData)
parts.push('accessibility'); if (parts.length === 0) parts.push('functional');
then set runTypesLabel = parts.join(' + '), updating the logic around
runTypesLabel, parts, hasPerformanceData, and hasAccessibilityData.
In `@scripts/merge-qa-reports.sh`:
- Around line 108-110: Sanitize the website-derived filename before building
REPORT_FILE: first strip any path components from WEBSITE_NAME (use
basename-like handling), then normalize to uppercase and replace any
non-alphanumeric characters with underscores (so WEBSITE_NAME_UPPER contains
only A-Z, 0-9 and underscores), trim repeated/leading underscores and fall back
to a safe default if the result is empty; then build REPORT_FILE using
REPORTS_DIR, WEBSITE_NAME_UPPER and REPORT_DATE. Update the assignments that
compute WEBSITE_NAME_UPPER and REPORT_FILE (references: WEBSITE_NAME,
WEBSITE_NAME_UPPER, REPORT_DATE, REPORT_FILE, REPORTS_DIR) to perform these
defensive sanitization steps.
In `@scripts/run-qa-report.sh`:
- Around line 100-102: REPORT_FILE can be malformed if WEBSITE_NAME_UPPER
contains path-significant characters (e.g., '/'), so sanitize WEBSITE_NAME_UPPER
before constructing REPORT_FILE and before any existence checks; update the code
that sets REPORT_FILE to use a sanitized version of WEBSITE_NAME_UPPER (or
create a new variable like SANITIZED_WEBSITE_NAME) by stripping or replacing
characters outside a safe whitelist (e.g., convert spaces and all
non-alphanumeric/underscore/dash/dot chars to underscores) using POSIX-safe
shell ops (parameter expansion or tr), and then use that sanitized identifier
when building REPORT_FILE and when referencing the file for existence or
generation checks.
In `@skills/performance/SKILL.md`:
- Line 313: Update the SKILL.md JSON example for the metric field to remove the
instructional "Optional:" prefix and use a realistic value that matches the
qa-report-performance-schema.json style (e.g., change `"metric": "Optional:
specific measurement, e.g. Load time 4200ms"` to `"metric": "Load time 4200ms"`
or to `"metric": "Load time >3s"` depending on which format the schema
`qa-report-performance-schema.json` expects), ensuring the `metric` example
value in the skill documentation and the schema are consistent.
---
Outside diff comments:
In `@scripts/merge-qa-reports.js`:
- Around line 107-116: The merge currently drops issue.screenshots causing
missing evidence in merged reports; update the object pushed into merged (the
block that builds the merged entry in scripts/merge-qa-reports.js) to include
screenshots when present (e.g., add a conditional spread for issue.screenshots
similar to metric and wcag_criterion) so merged entries preserve the screenshots
array for downstream HTML rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6722598d-e7a2-4f5a-878c-b49019a05b7a
📒 Files selected for processing (14)
.gitignoreCLAUDE.mdREADME.mddocs/getting-started.mdschemas/qa-report-accessibility-schema.jsonschemas/qa-report-functional-schema.jsonschemas/qa-report-performance-schema.jsonscripts/generate-report.jsscripts/merge-qa-reports.jsscripts/merge-qa-reports.shscripts/run-qa-report.shskills/a11y/SKILL.mdskills/functional-design/SKILL.mdskills/performance/SKILL.md
Comment on lines
+81
to
+84
| const parts = ['functional']; | ||
| if (hasPerformanceData) parts.push('performance'); | ||
| if (hasAccessibilityData) parts.push('accessibility'); | ||
| runTypesLabel = parts.join(' + '); |
There was a problem hiding this comment.
runTypesLabel currently over-reports functional coverage when no flag is passed.
The fallback path always seeds parts with "functional", so non-functional single-run inputs are mislabeled.
Suggested patch
- const parts = ['functional'];
+ const hasFunctionalData = Boolean(
+ report.links ||
+ report.testMethodology ||
+ (Array.isArray(report.visitedPages) && report.visitedPages.length > 0)
+ );
+ const parts = [];
+ if (hasFunctionalData || (!hasPerformanceData && !hasAccessibilityData)) parts.push('functional');
if (hasPerformanceData) parts.push('performance');
if (hasAccessibilityData) parts.push('accessibility');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const parts = ['functional']; | |
| if (hasPerformanceData) parts.push('performance'); | |
| if (hasAccessibilityData) parts.push('accessibility'); | |
| runTypesLabel = parts.join(' + '); | |
| const hasFunctionalData = Boolean( | |
| report.links || | |
| report.testMethodology || | |
| (Array.isArray(report.visitedPages) && report.visitedPages.length > 0) | |
| ); | |
| const parts = []; | |
| if (hasFunctionalData || (!hasPerformanceData && !hasAccessibilityData)) parts.push('functional'); | |
| if (hasPerformanceData) parts.push('performance'); | |
| if (hasAccessibilityData) parts.push('accessibility'); | |
| runTypesLabel = parts.join(' + '); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/generate-report.js` around lines 81 - 84, Initialize parts as an
empty array instead of seeding it with 'functional' and only add 'functional'
when neither hasPerformanceData nor hasAccessibilityData is true; specifically,
replace the current parts initialization and pushes so that parts = []; if
(hasPerformanceData) parts.push('performance'); if (hasAccessibilityData)
parts.push('accessibility'); if (parts.length === 0) parts.push('functional');
then set runTypesLabel = parts.join(' + '), updating the logic around
runTypesLabel, parts, hasPerformanceData, and hasAccessibilityData.
| .map((relPath) => { | ||
| const safePath = escAttr(relPath); | ||
| const filename = relPath.split('/').pop(); | ||
| return `<figure class="screenshot"><a href="${safePath}" target="_blank"><img src="${safePath}" alt="${escAttr(filename)}" loading="lazy"></a><figcaption>${escHtml(filename)}</figcaption></figure>`; |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to all target="_blank" links.
Opening external links in new tabs without rel="noopener noreferrer" exposes window.opener and allows reverse-tabnabbing.
Suggested patch
- return `<figure class="screenshot"><a href="${safePath}" target="_blank"><img src="${safePath}" alt="${escAttr(filename)}" loading="lazy"></a><figcaption>${escHtml(filename)}</figcaption></figure>`;
+ return `<figure class="screenshot"><a href="${safePath}" target="_blank" rel="noopener noreferrer"><img src="${safePath}" alt="${escAttr(filename)}" loading="lazy"></a><figcaption>${escHtml(filename)}</figcaption></figure>`;- .map((p) => `<li><a href="${escAttr(p)}" target="_blank">${escHtml(p)}</a></li>`)
+ .map((p) => `<li><a href="${escAttr(p)}" target="_blank" rel="noopener noreferrer">${escHtml(p)}</a></li>`)- ? `<ul>${visitedPages.map((p) => `<li><a href="${escAttr(p)}" target="_blank">${escHtml(p)}</a></li>`).join('')}</ul>`
+ ? `<ul>${visitedPages.map((p) => `<li><a href="${escAttr(p)}" target="_blank" rel="noopener noreferrer">${escHtml(p)}</a></li>`).join('')}</ul>`- <p class="site-url"><a href="${escAttr(report.url)}" target="_blank">${escHtml(report.url)}</a></p>
+ <p class="site-url"><a href="${escAttr(report.url)}" target="_blank" rel="noopener noreferrer">${escHtml(report.url)}</a></p>Also applies to: 102-102, 159-159, 477-477
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/generate-report.js` at line 93, Update the anchor tags that open in a
new tab to include rel="noopener noreferrer": locate the template that returns
the figure HTML (the expression using safePath, escAttr(filename), and
escHtml(filename) which currently contains <a href="${safePath}"
target="_blank">) and add rel="noopener noreferrer" to that <a> element; do the
same for the other similar templates that render links (the other occurrences
that use target="_blank" around safePath/filename) so all external new-tab links
include rel="noopener noreferrer".
Comment on lines
+108
to
+110
| WEBSITE_NAME_UPPER=$(echo "$WEBSITE_NAME" | tr '[:lower:]' '[:upper:]' | tr ' ' '_') | ||
| REPORT_DATE=$(echo "$TIMESTAMP" | cut -d'T' -f1) | ||
| REPORT_FILE="$REPORTS_DIR/${WEBSITE_NAME_UPPER}_QA_REPORT_${REPORT_DATE}.md" | ||
| REPORT_FILE="$REPORTS_DIR/${WEBSITE_NAME_UPPER}_QA_REPORT_${REPORT_DATE}.html" |
There was a problem hiding this comment.
Sanitize website-derived filename more defensively.
Line 108 only handles spaces. A website name containing path separators or other special characters can produce an invalid REPORT_FILE path at Line 110 and trigger a false “not generated” error.
Suggested patch
-WEBSITE_NAME_UPPER=$(echo "$WEBSITE_NAME" | tr '[:lower:]' '[:upper:]' | tr ' ' '_')
+WEBSITE_NAME_UPPER=$(echo "$WEBSITE_NAME" | tr '[:lower:]' '[:upper:]' | sed -E 's/[^A-Z0-9]+/_/g; s/^_+|_+$//g')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| WEBSITE_NAME_UPPER=$(echo "$WEBSITE_NAME" | tr '[:lower:]' '[:upper:]' | tr ' ' '_') | |
| REPORT_DATE=$(echo "$TIMESTAMP" | cut -d'T' -f1) | |
| REPORT_FILE="$REPORTS_DIR/${WEBSITE_NAME_UPPER}_QA_REPORT_${REPORT_DATE}.md" | |
| REPORT_FILE="$REPORTS_DIR/${WEBSITE_NAME_UPPER}_QA_REPORT_${REPORT_DATE}.html" | |
| WEBSITE_NAME_UPPER=$(echo "$WEBSITE_NAME" | tr '[:lower:]' '[:upper:]' | sed -E 's/[^A-Z0-9]+/_/g; s/^_+|_+$//g') | |
| REPORT_DATE=$(echo "$TIMESTAMP" | cut -d'T' -f1) | |
| REPORT_FILE="$REPORTS_DIR/${WEBSITE_NAME_UPPER}_QA_REPORT_${REPORT_DATE}.html" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/merge-qa-reports.sh` around lines 108 - 110, Sanitize the
website-derived filename before building REPORT_FILE: first strip any path
components from WEBSITE_NAME (use basename-like handling), then normalize to
uppercase and replace any non-alphanumeric characters with underscores (so
WEBSITE_NAME_UPPER contains only A-Z, 0-9 and underscores), trim
repeated/leading underscores and fall back to a safe default if the result is
empty; then build REPORT_FILE using REPORTS_DIR, WEBSITE_NAME_UPPER and
REPORT_DATE. Update the assignments that compute WEBSITE_NAME_UPPER and
REPORT_FILE (references: WEBSITE_NAME, WEBSITE_NAME_UPPER, REPORT_DATE,
REPORT_FILE, REPORTS_DIR) to perform these defensive sanitization steps.
Comment on lines
+100
to
+102
| REPORT_FILE="$REPORTS_DIR/${WEBSITE_NAME_UPPER}_${TEST_TYPE_LABEL}_QA_REPORT_${REPORT_DATE}.html" | ||
| else | ||
| REPORT_FILE="$REPORTS_DIR/${WEBSITE_NAME_UPPER}_QA_REPORT_${REPORT_DATE}.md" | ||
| REPORT_FILE="$REPORTS_DIR/${WEBSITE_NAME_UPPER}_QA_REPORT_${REPORT_DATE}.html" |
There was a problem hiding this comment.
Harden report filename sanitization before existence check.
Line 88 only normalizes spaces. If websiteName includes / (or other path-significant chars), REPORT_FILE can be malformed and the script may report a false failure even when HTML generation succeeded.
Suggested patch
-WEBSITE_NAME_UPPER=$(echo "$WEBSITE_NAME" | tr '[:lower:]' '[:upper:]' | tr ' ' '_')
+WEBSITE_NAME_UPPER=$(echo "$WEBSITE_NAME" | tr '[:lower:]' '[:upper:]' | sed -E 's/[^A-Z0-9]+/_/g; s/^_+|_+$//g')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/run-qa-report.sh` around lines 100 - 102, REPORT_FILE can be
malformed if WEBSITE_NAME_UPPER contains path-significant characters (e.g.,
'/'), so sanitize WEBSITE_NAME_UPPER before constructing REPORT_FILE and before
any existence checks; update the code that sets REPORT_FILE to use a sanitized
version of WEBSITE_NAME_UPPER (or create a new variable like
SANITIZED_WEBSITE_NAME) by stripping or replacing characters outside a safe
whitelist (e.g., convert spaces and all non-alphanumeric/underscore/dash/dot
chars to underscores) using POSIX-safe shell ops (parameter expansion or tr),
and then use that sanitized identifier when building REPORT_FILE and when
referencing the file for existence or generation checks.
| "device": "mobile|desktop|both", | ||
| "pages": ["https://example.com/page"], | ||
| "metric": "Optional: specific measurement, e.g. Load time 4200ms" | ||
| "metric": "Optional: specific measurement, e.g. Load time 4200ms", |
There was a problem hiding this comment.
Schema-skill documentation inconsistency on the metric field example.
The schema at schemas/qa-report-performance-schema.json line 39 shows "metric": "Load time >3s" while this skill documentation shows "metric": "Optional: specific measurement, e.g. Load time 4200ms". The "Optional:" prefix in the skill example is instructional commentary rather than a realistic field value. As per coding guidelines, schema files must define the structure of JSON reports that match what the corresponding skill collects.
Consider aligning the skill's JSON example to show a realistic metric value (e.g., "metric": "Load time 4200ms") matching the schema's style, or update the schema to match this format.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/performance/SKILL.md` at line 313, Update the SKILL.md JSON example
for the metric field to remove the instructional "Optional:" prefix and use a
realistic value that matches the qa-report-performance-schema.json style (e.g.,
change `"metric": "Optional: specific measurement, e.g. Load time 4200ms"` to
`"metric": "Load time 4200ms"` or to `"metric": "Load time >3s"` depending on
which format the schema `qa-report-performance-schema.json` expects), ensuring
the `metric` example value in the skill documentation and the schema are
consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
why
Summary by CodeRabbit
New Features
Documentation