feat: a11y - #13561
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR establishes end-to-end accessibility testing and audit infrastructure. It adds an IBM accessibility-checker configuration, Playwright scanning fixtures, Jest-Axe unit test support, GitHub Actions automation, and comprehensive documentation outlining current compliance gaps and a phased five-phase remediation plan. ChangesAccessibility Testing & Audit Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (6 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 |
- Added `accessibility-checker` package to the project dependencies. - Enhanced test fixtures to include accessibility scanning capabilities. - Implemented `runA11yScan` method in the `LangflowPage` type for running accessibility checks. - Created utility functions for building accessibility scan labels, formatting failure messages, and generating summary attachments. - Updated test setup to manage accessibility sessions and assert compliance based on scan results.
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-1.11.0 #13561 +/- ##
==================================================
+ Coverage 59.22% 59.49% +0.26%
==================================================
Files 2334 2291 -43
Lines 223414 222371 -1043
Branches 31221 32770 +1549
==================================================
- Hits 132328 132306 -22
+ Misses 89572 88551 -1021
Partials 1514 1514
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
a11y-gap-report.md (1)
51-51: 💤 Low valueConsider rewording "NEEDS TARGETED AUDIT" for clarity.
The phrase reads awkwardly. Consider alternatives:
- "REQUIRES TARGETED AUDIT"
- "NEEDS A TARGETED AUDIT"
- "TO BE AUDITED"
Also applies to: 169-169
🤖 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 `@a11y-gap-report.md` at line 51, The table cell text "NEEDS TARGETED AUDIT" is awkward; replace it with a clearer phrase like "REQUIRES TARGETED AUDIT" (or "NEEDS A TARGETED AUDIT"/"TO BE AUDITED") in the two occurrences found for the Error Prevention criterion (`3.3.4`) and the other occurrence referenced at the second location; update both instances so the table entries consistently use the chosen phrasing.Source: Linters/SAST tools
.github/workflows/a11y-scan.yml (3)
38-41: ⚡ Quick winConsider setting
persist-credentials: falsefor security.The checkout step does not explicitly set
persist-credentials: false, which means the GitHub token remains available to subsequent steps. If the workflow never needs to push commits or access private repositories, setting this tofalsereduces the risk of credential exposure.🔒 Set persist-credentials to false
- name: Checkout Repository uses: actions/checkout@v6 with: ref: ${{ inputs.ref || github.ref }} + persist-credentials: false🤖 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 @.github/workflows/a11y-scan.yml around lines 38 - 41, Update the Checkout Repository step (uses: actions/checkout@v6) to explicitly set persist-credentials: false so the GITHUB_TOKEN is not left available to later steps; locate the checkout step in the workflow and add the persist-credentials: false key under its with: block to disable credential persistence when no further pushes or private-repo access are required.
114-119: ⚡ Quick winAvoid running aggregation command twice.
The
npm run a11y:report --silentcommand is executed twice: once at line 115 (output discarded) and again at line 118 (captured for summary). Instead, capture the output once and reuse it:♻️ Capture output once and reuse
cd src/frontend if ls coverage/accessibility-reports/*.json > /dev/null 2>&1; then - npm run a11y:report --silent + REPORT_OUTPUT=$(npm run a11y:report --silent) { echo '```text' - npm run a11y:report --silent + echo "$REPORT_OUTPUT" echo '```' } >> "$GITHUB_STEP_SUMMARY" else🤖 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 @.github/workflows/a11y-scan.yml around lines 114 - 119, The workflow currently runs npm run a11y:report --silent twice; capture its output once into a variable (e.g., REPORT_OUTPUT="$(npm run a11y:report --silent)") before the summary block and then reuse that variable when appending to $GITHUB_STEP_SUMMARY (echo "$REPORT_OUTPUT") instead of calling npm run a11y:report --silent again; update the block around the ls check and the code that appends to GITHUB_STEP_SUMMARY to use REPORT_OUTPUT and remove the duplicate command invocation.
39-39: ⚖️ Poor tradeoffConsider pinning GitHub Actions to commit SHAs.
The workflow uses semantic version tags (
@v6,@v5) for actions. Some security policies require pinning to commit SHAs to prevent tag manipulation. If your organization follows this policy, consider pinning actions to their commit hashes.Example:
uses: actions/checkout@<commit-sha> # instead of `@v6`This is a trade-off between security and maintainability — semantic versions are easier to update but commit SHAs provide stronger immutability guarantees.
Also applies to: 44-44, 54-54, 68-68, 84-84, 127-127
🤖 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 @.github/workflows/a11y-scan.yml at line 39, Replace the semantic version tags with pinned commit SHAs for each GitHub Action usage to satisfy immutability/security requirements: update uses entries for actions/checkout@v6, actions/setup-node@v18, github/super-linter@v5, actions/upload-artifact@v4, actions/cache@v4 (and any other uses entries in this workflow) to the corresponding commit SHA for the desired release; fetch the exact commit SHA from the action repo (or release tag) and substitute the `@tag` with @<commit-sha> so each uses: line is pinned to an immutable commit.
🤖 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 @.github/workflows/a11y-scan.yml:
- Around line 4-7: Remove the temporary push trigger that targets the specific
branch by deleting the "push: branches: [feat/a11y]" block (the temporary
trigger) from the workflow so the job relies on the intended triggers (e.g.,
workflow_dispatch and any global triggers); ensure only the permanent triggers
remain and no branch-specific test trigger referencing "feat/a11y" is left in
the a11y-scan workflow.
In `@a11y-gap-report.md`:
- Line 7: Replace all hard-coded absolute paths in documentation with
repository-relative paths: update occurrences of
"/Users/viktoravelino/projects/langflow/a11y-level-1-requirements.md" to
"a11y-level-1-requirements.md",
"/Users/viktoravelino/projects/langflow/a11y-gap-report.md" to
"a11y-gap-report.md", and any
"/Users/viktoravelino/projects/langflow/src/frontend/..." references to
"src/frontend/..." in files such as a11y-gap-report.md and a11y-action-plan.md;
ensure you search for and replace these absolute path patterns across the docs
so links to a11y-level-1-requirements.md, a11y-gap-report.md, and
src/frontend/... are repository-relative and work in CI and for other
developers.
In `@a11y-test-usage.md`:
- Around line 14-16: The three file links in a11y-test-usage.md currently use
absolute local paths; update the entries that reference
src/frontend/tests/fixtures.ts,
src/frontend/tests/utils/accessibility-checker.ts, and
src/frontend/.achecker.yml to use repository-relative paths (e.g., remove the
leading /Users/viktoravelino/projects/langflow/ prefix so the links point to
src/frontend/tests/fixtures.ts,
src/frontend/tests/utils/accessibility-checker.ts, and
src/frontend/.achecker.yml). Make the edit in the lines containing those links
so they work for other developers and CI.
In `@src/frontend/src/components/ui/__tests__/checkbox.a11y.test.tsx`:
- Around line 1-35: This Jest/RTL test file uses react-testing-library but must
follow the repo's Playwright frontend test standard: replace the two describe
blocks testing Checkbox and CheckBoxDiv with Playwright tests (or move/rename
this file out of the *.test.tsx frontend pattern if you intend to keep Jest).
Specifically, create Playwright tests that mount/render the Checkbox and
CheckBoxDiv components and assert (1) no a11y violations for Checkbox (use your
repo's axe/playwright helper or Playwright accessibility checks) and (2) that
Checkbox exposes role "checkbox" and is checked when passed checked, referencing
the Checkbox component; for the CheckBoxDiv case use Playwright's test.fail (or
mark as expected failure) and assert that it currently does not expose a
checkbox role/state, referencing CheckBoxDiv. Ensure the new tests live under
the Playwright test directory/naming convention (*.spec.ts or *.spec.tsx) and
remove or rename the original Jest file so it no longer matches the frontend
*.test.tsx pattern.
---
Nitpick comments:
In @.github/workflows/a11y-scan.yml:
- Around line 38-41: Update the Checkout Repository step (uses:
actions/checkout@v6) to explicitly set persist-credentials: false so the
GITHUB_TOKEN is not left available to later steps; locate the checkout step in
the workflow and add the persist-credentials: false key under its with: block to
disable credential persistence when no further pushes or private-repo access are
required.
- Around line 114-119: The workflow currently runs npm run a11y:report --silent
twice; capture its output once into a variable (e.g., REPORT_OUTPUT="$(npm run
a11y:report --silent)") before the summary block and then reuse that variable
when appending to $GITHUB_STEP_SUMMARY (echo "$REPORT_OUTPUT") instead of
calling npm run a11y:report --silent again; update the block around the ls check
and the code that appends to GITHUB_STEP_SUMMARY to use REPORT_OUTPUT and remove
the duplicate command invocation.
- Line 39: Replace the semantic version tags with pinned commit SHAs for each
GitHub Action usage to satisfy immutability/security requirements: update uses
entries for actions/checkout@v6, actions/setup-node@v18, github/super-linter@v5,
actions/upload-artifact@v4, actions/cache@v4 (and any other uses entries in this
workflow) to the corresponding commit SHA for the desired release; fetch the
exact commit SHA from the action repo (or release tag) and substitute the `@tag`
with @<commit-sha> so each uses: line is pinned to an immutable commit.
In `@a11y-gap-report.md`:
- Line 51: The table cell text "NEEDS TARGETED AUDIT" is awkward; replace it
with a clearer phrase like "REQUIRES TARGETED AUDIT" (or "NEEDS A TARGETED
AUDIT"/"TO BE AUDITED") in the two occurrences found for the Error Prevention
criterion (`3.3.4`) and the other occurrence referenced at the second location;
update both instances so the table entries consistently use the chosen phrasing.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d0bf27e-33ac-4160-9739-45ad5f7cecaf
⛔ Files ignored due to path filters (1)
src/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
.github/workflows/a11y-scan.ymla11y-action-plan.mda11y-gap-report.mda11y-level-1-requirements.mda11y-test-usage.mddocker/build_and_push.Dockerfiledocker/build_and_push_ep.Dockerfiledocker/build_and_push_with_extras.Dockerfilesrc/frontend/.achecker.ymlsrc/frontend/package.jsonsrc/frontend/src/components/ui/__tests__/checkbox.a11y.test.tsxsrc/frontend/src/setupTests.tssrc/frontend/src/utils/a11y-test.tssrc/frontend/tests/core/unit/chatInputOutput.spec.tssrc/frontend/tests/fixtures.tssrc/frontend/tests/utils/accessibility-checker.tssrc/frontend/tests/utils/aggregate-a11y-reports.mjssrc/frontend/tests/utils/types.ts
Adam-Aghili
left a comment
There was a problem hiding this comment.
Change base from 1.10.0 to 1.11.0
Adam-Aghili
left a comment
There was a problem hiding this comment.
Didn't look through every spec file but this is a pretty good first steps
This is possibly out of scope for this PR because the way we do dark/light mode is kind of inconsistent doesn't allow for this but can we have the fixture always go through both instead of having to repeatedly define dark/light mode in each test?
or even have it be like
await page.runA11yScan("homepage", { colorScheme: "light" });
await page.runA11yScan("homepage", { colorScheme: "dark" });
* feat: add accessibility scan reports * docs: add a11y scan route map * test(a11y): add manifest-backed scans * ci(a11y): summarize scans by route
Adam-Aghili
left a comment
There was a problem hiding this comment.
LGTM it is up to you if you rather fix the spcs to use the new runA11yScan options now or fix it as you go.
We will fix as we go, a lot of the specs are going to be redone. I dont want to bloat this PR. |
Summary by CodeRabbit
Release Notes
New Features
runA11yScan()helper for manual accessibility checks during test execution.Documentation
Chores