fix(a11y): reduce jsx-a11y warnings common 18→8, studio 45→31#446
Open
marcusds wants to merge 2 commits into
Open
fix(a11y): reduce jsx-a11y warnings common 18→8, studio 45→31#446marcusds wants to merge 2 commits into
marcusds wants to merge 2 commits into
Conversation
- Replace role="button" divs/spans with native <button> elements - Fix form label associations with aria-labelledby pattern - Add keyboard event handlers where missing - Wrap emojis with proper ARIA in test files - Remove redundant role="form" on native form element - Add aria-hidden + tabIndex=-1 on decorative canvas - Create update-max-warnings-a11y.ts script for maintaining baselines
marcusds
force-pushed
the
astd-266-fix-critical-accessibility-issues-identified-by-jsx-a11y-lint/mschwab
branch
from
639c414 to
d34834e
Compare
📝 WalkthroughWalkthroughThe PR lowers accessibility lint warning thresholds in the common and studio web packages, adds scripts to regenerate those thresholds, and updates multiple common and studio components and test mocks to use native buttons, hidden decorative elements, and labeled form controls. ChangesWeb accessibility and UI semantics
Sequence Diagram(s)sequenceDiagram
participant update_max_warnings_a11y_ts as "update-max-warnings-a11y.ts"
participant pnpm_exec_eslint as "pnpm exec eslint"
participant package_json as "package.json"
update_max_warnings_a11y_ts->>pnpm_exec_eslint: run ESLint with the a11y config
pnpm_exec_eslint-->>update_max_warnings_a11y_ts: JSON warningCount results
update_max_warnings_a11y_ts->>package_json: read lint:a11y --max-warnings
update_max_warnings_a11y_ts->>package_json: write the updated threshold
Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@web/packages/common/scripts/update-max-warnings-a11y.ts`:
- Around line 35-40: The update flow in update-max-warnings-a11y.ts assumes
lint:a11y always contains a --max-warnings value, so a changed script format can
silently skip replacement while still writing the package as if it were updated.
In the logic around maxWarningsRegex, currentMax, and the replace on
pkg.scripts['lint:a11y'], add a guard that verifies the regex matches before
attempting to rewrite; if it does not, fail fast with an explicit error instead
of continuing. Keep the existing warningCount comparison and only write pkgPath
after a successful, verified replacement.
In `@web/packages/studio/src/components/FileTag/index.tsx`:
- Around line 67-73: The FileTag button currently renders as an active,
focusable control even when onNoFileClick is missing. Update the button in
FileTag to disable it when no handler is provided by using
disabled={!onNoFileClick}, and add aria-disabled if needed for accessibility.
Keep the existing onClick behavior tied to onNoFileClick so the control only
appears interactive when it actually does something.
🪄 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: Enterprise
Run ID: 7e363d4a-6d24-4525-be44-95706f8b9b49
📒 Files selected for processing (16)
web/packages/common/package.jsonweb/packages/common/scripts/update-max-warnings-a11y.tsweb/packages/common/src/components/DataView/StudioAppliedFilters.test.tsxweb/packages/common/src/components/DataView/StudioDataView.test.tsxweb/packages/common/src/components/DataView/useRowClick.tsxweb/packages/common/src/components/Nebula/index.tsxweb/packages/common/src/components/StatusBadge/index.tsxweb/packages/common/src/components/TableEmptyState/TableEmptyState.test.tsxweb/packages/common/src/components/UploadModal/SimpleFilesTable.tsxweb/packages/common/src/components/form/ControlledSearchableSelect/index.test.tsxweb/packages/studio/package.jsonweb/packages/studio/src/components/FileTag/index.tsxweb/packages/studio/src/components/FilesTable/index.tsxweb/packages/studio/src/components/NewCustomizationForm/index.tsxweb/packages/studio/src/routes/SafeSynthesizerNewRoute/components/AdvancedParameters.tsxweb/packages/studio/src/tests/util/mockStudioDataView.tsx
Comment on lines
+35
to
+40
| const maxWarningsRegex = /--max-warnings (\d+)/; | ||
| const currentMax: number = parseInt(a11yScript.match(maxWarningsRegex)?.[1] || '0', 10); | ||
|
|
||
| if (warningCount !== currentMax) { | ||
| pkg.scripts['lint:a11y'] = a11yScript.replace(maxWarningsRegex, `--max-warnings ${warningCount}`); | ||
| fs.writeFileSync(pkgPath, JSON.stringify(pkg, null, 2) + '\n'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard missing --max-warnings match before rewrite.
If lint:a11y format changes, replace() does nothing and the script can report a false “updated” state. Fail fast when the flag is absent.
Proposed fix
const maxWarningsRegex = /--max-warnings (\d+)/;
-const currentMax: number = parseInt(a11yScript.match(maxWarningsRegex)?.[1] || '0', 10);
+const maxWarningsMatch = a11yScript.match(maxWarningsRegex);
+if (!maxWarningsMatch) {
+ console.error('lint:a11y script is missing --max-warnings <number>');
+ process.exit(1);
+}
+const currentMax: number = parseInt(maxWarningsMatch[1], 10);
if (warningCount !== currentMax) {
pkg.scripts['lint:a11y'] = a11yScript.replace(maxWarningsRegex, `--max-warnings ${warningCount}`);📝 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 maxWarningsRegex = /--max-warnings (\d+)/; | |
| const currentMax: number = parseInt(a11yScript.match(maxWarningsRegex)?.[1] || '0', 10); | |
| if (warningCount !== currentMax) { | |
| pkg.scripts['lint:a11y'] = a11yScript.replace(maxWarningsRegex, `--max-warnings ${warningCount}`); | |
| fs.writeFileSync(pkgPath, JSON.stringify(pkg, null, 2) + '\n'); | |
| const maxWarningsRegex = /--max-warnings (\d+)/; | |
| const maxWarningsMatch = a11yScript.match(maxWarningsRegex); | |
| if (!maxWarningsMatch) { | |
| console.error('lint:a11y script is missing --max-warnings <number>'); | |
| process.exit(1); | |
| } | |
| const currentMax: number = parseInt(maxWarningsMatch[1], 10); | |
| if (warningCount !== currentMax) { | |
| pkg.scripts['lint:a11y'] = a11yScript.replace(maxWarningsRegex, `--max-warnings ${warningCount}`); | |
| fs.writeFileSync(pkgPath, JSON.stringify(pkg, null, 2) + '\n'); |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 39-39: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(pkgPath, JSON.stringify(pkg, null, 2) + '\n')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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 `@web/packages/common/scripts/update-max-warnings-a11y.ts` around lines 35 -
40, The update flow in update-max-warnings-a11y.ts assumes lint:a11y always
contains a --max-warnings value, so a changed script format can silently skip
replacement while still writing the package as if it were updated. In the logic
around maxWarningsRegex, currentMax, and the replace on
pkg.scripts['lint:a11y'], add a guard that verifies the regex matches before
attempting to rewrite; if it does not, fail fast with an explicit error instead
of continuing. Keep the existing warningCount comparison and only write pkgPath
after a successful, verified replacement.
Comment on lines
+67
to
+73
| <button | ||
| type="button" | ||
| onClick={onNoFileClick} | ||
| className={onNoFileClick ? 'cursor-pointer' : undefined} | ||
| > | ||
| {missingFileNameChip} | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Disable button when no click handler.
When onNoFileClick is absent, this still renders a focusable button with no action. Set disabled={!onNoFileClick} (and optional aria-disabled) to avoid a dead-end control.
🤖 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 `@web/packages/studio/src/components/FileTag/index.tsx` around lines 67 - 73,
The FileTag button currently renders as an active, focusable control even when
onNoFileClick is missing. Update the button in FileTag to disable it when no
handler is provided by using disabled={!onNoFileClick}, and add aria-disabled if
needed for accessibility. Keep the existing onClick behavior tied to
onNoFileClick so the control only appears interactive when it actually does
something.
- Replace div role="button" with native button in ReportTraceModal - Replace Anchor role="button" with Button in FilesetFilePreviewLink - Add aria-label to hidden file input in AddToolForm - Add aria-label to mock textarea in DatasetSchemaEditor test - Remove role="menu" from ul in mockStudioDataView - Simplify FilesTable quick actions wrapper - Remove span role="img" from emoji test data - Remove event handlers from mock table in StudioDataView test
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/studio/src/components/ReportTraceModal/index.tsx (1)
226-262: 🩺 Stability & Availability | 🟠 MajorMove the Copy action out of the row
<button>. The nested<Button>inside the outer<button>is invalid HTML and breaks keyboard/screen-reader semantics. Use a non-button wrapper with explicit keyboard handling, or place Copy outside the clickable row.🤖 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 `@web/packages/studio/src/components/ReportTraceModal/index.tsx` around lines 226 - 262, The trace row in ReportTraceModal is using a nested interactive control: the inner Copy <Button> sits inside the outer row <button>, which creates invalid semantics and accessibility issues. Update the trace item rendering in the trace list so the row click target and the copy action are separate controls; keep handleViewDetails on the row container, and move handleCopyTrace to a standalone action outside that button or replace the row button with a non-button wrapper that has explicit keyboard support. Use the ReportTraceModal trace item markup, handleViewDetails, and handleCopyTrace to locate and adjust this interaction.
🤖 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 `@web/packages/studio/src/components/FilesTable/index.tsx`:
- Around line 79-83: Quick-action clicks are still bubbling up and triggering
the row action, so opening or using the file/directory quick actions can select
or navigate the row. Update the quick-actions handling in FileQuickActions,
DirectoryQuickActions, or QuickActionsMenuRoot so click propagation is stopped
within the actions/menu control itself rather than relying on the row cell
wrapper, and keep the row activation logic in the table row unchanged.
In
`@web/packages/studio/src/components/SafeSynthesizerFilesetPreview/FilesetFilePreviewLink.tsx`:
- Around line 135-141: The preview button in FilesetFilePreviewLink can render
without an accessible label when children is missing, so update the Button
content to fall back to the formatted file path used by the component. Use the
existing FilesetFilePreviewLink rendering logic and its path-formatting
helper/state to ensure the button always shows a label, with children taking
precedence only when provided.
---
Outside diff comments:
In `@web/packages/studio/src/components/ReportTraceModal/index.tsx`:
- Around line 226-262: The trace row in ReportTraceModal is using a nested
interactive control: the inner Copy <Button> sits inside the outer row <button>,
which creates invalid semantics and accessibility issues. Update the trace item
rendering in the trace list so the row click target and the copy action are
separate controls; keep handleViewDetails on the row container, and move
handleCopyTrace to a standalone action outside that button or replace the row
button with a non-button wrapper that has explicit keyboard support. Use the
ReportTraceModal trace item markup, handleViewDetails, and handleCopyTrace to
locate and adjust this interaction.
🪄 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: Enterprise
Run ID: e05bd2d8-925f-4156-93c4-7622bf224f0f
📒 Files selected for processing (9)
web/packages/common/package.jsonweb/packages/common/src/components/DataView/StudioDataView.test.tsxweb/packages/studio/package.jsonweb/packages/studio/src/components/FilesTable/index.tsxweb/packages/studio/src/components/PromptTuningForm/ToolsSection/components/AddToolForm.tsxweb/packages/studio/src/components/ReportTraceModal/index.tsxweb/packages/studio/src/components/SafeSynthesizerFilesetPreview/FilesetFilePreviewLink.tsxweb/packages/studio/src/routes/FilesetDetailRoute/DatasetSchemaEditor/index.test.tsxweb/packages/studio/src/tests/util/mockStudioDataView.tsx
✅ Files skipped from review due to trivial changes (2)
- web/packages/studio/src/routes/FilesetDetailRoute/DatasetSchemaEditor/index.test.tsx
- web/packages/studio/src/components/PromptTuningForm/ToolsSection/components/AddToolForm.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/packages/studio/src/tests/util/mockStudioDataView.tsx
- web/packages/common/package.json
Comment on lines
+79
to
+83
| file.type === 'file' ? ( | ||
| <FileQuickActions file={file} datasetId={datasetFullName} /> | ||
| ) : ( | ||
| <DirectoryQuickActions directory={file} datasetId={datasetFullName} /> | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent quick-action clicks from selecting the row.
Line 79 removes the propagation boundary while Line 93 still activates rows. Native buttons/menu items bubble clicks, so opening or choosing quick actions can also navigate/open the row unless QuickActionsMenuRoot handles this internally. Move propagation isolation into the quick-actions/menu control or the actions cell.
🤖 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 `@web/packages/studio/src/components/FilesTable/index.tsx` around lines 79 -
83, Quick-action clicks are still bubbling up and triggering the row action, so
opening or using the file/directory quick actions can select or navigate the
row. Update the quick-actions handling in FileQuickActions,
DirectoryQuickActions, or QuickActionsMenuRoot so click propagation is stopped
within the actions/menu control itself rather than relying on the row cell
wrapper, and keep the row activation logic in the table row unchanged.
Comment on lines
+135
to
+141
| <Button | ||
| kind="tertiary" | ||
| onClick={handlePreviewClick} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault(); | ||
| handlePreviewClick(); | ||
| } | ||
| }} | ||
| className="truncate max-w-full cursor-pointer" | ||
| className="truncate max-w-full text-left p-0 h-auto" | ||
| > | ||
| {children} | ||
| </Anchor> | ||
| </Button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if a formatted-path fallback existed/exists and how callers pass children
fd FilesetFilePreviewLink.tsx --exec cat -n {}
rg -nP 'FilesetFilePreviewLink' --type=tsx -C2Repository: NVIDIA-NeMo/nemo-platform
Length of output: 5889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect this component and its callers for children usage / fallback behavior.
git ls-files 'web/packages/studio/src/components/SafeSynthesizerFilesetPreview/FilesetFilePreviewLink.tsx' \
'web/packages/studio/src/components/SafeSynthesizerFilesetPreview/*' \
'web/packages/studio/src/**' \
| sed -n '1,200p'
printf '\n--- usages ---\n'
rg -n "FilesetFilePreviewLink" web/packages/studio/src -g '*.ts' -g '*.tsx' -C 3
printf '\n--- children prop patterns ---\n'
rg -n "children\\?: ReactNode|children\\s*=|<FilesetFilePreviewLink" web/packages/studio/src -g '*.ts' -g '*.tsx' -C 2Repository: NVIDIA-NeMo/nemo-platform
Length of output: 33021
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map nearby files to find docs/comments about the fallback behavior.
ast-grep outline web/packages/studio/src/components/SafeSynthesizerFilesetPreview/FilesetFilePreviewLink.tsx --view expanded || true
printf '\n--- file excerpt around props and render ---\n'
sed -n '1,170p' web/packages/studio/src/components/SafeSynthesizerFilesetPreview/FilesetFilePreviewLink.tsxRepository: NVIDIA-NeMo/nemo-platform
Length of output: 5180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all call sites and inspect whether children are always provided.
rg -n "<FilesetFilePreviewLink\\b" web/packages/studio/src -g '*.tsx' -C 2Repository: NVIDIA-NeMo/nemo-platform
Length of output: 2230
Restore the formatted-path fallback
children is optional, so this button can render empty and lose its accessible name. Fall back to the formatted file path when no child is passed.
🤖 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
`@web/packages/studio/src/components/SafeSynthesizerFilesetPreview/FilesetFilePreviewLink.tsx`
around lines 135 - 141, The preview button in FilesetFilePreviewLink can render
without an accessible label when children is missing, so update the Button
content to fall back to the formatted file path used by the component. Use the
existing FilesetFilePreviewLink rendering logic and its path-formatting
helper/state to ensure the button always shows a label, with children taking
precedence only when provided.
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.
Fix critical accessibility issues identified by the jsx-a11y eslint plugin added in #431.
Warning reduction:
Key changes:
role="button"divs/spans with native<button>elements (useRowClick,FileTag, test mocks)aria-labelledbypattern (AdvancedParameters,SimpleFilesTable)FilesTable,mockStudioDataView)<span role="img" aria-label>in test filesrole="form"on native form elementaria-hidden+tabIndex=-1on decorative canvas (Nebula)update-max-warnings-a11y.tsscript for maintaining warning baselinesVerification
Fixes ASTD-266
Summary by CodeRabbit
New Features
Bug Fixes