fix(a11y): zero jsx-a11y warnings in non-test files + lint ratchet CI gate - #263
fix(a11y): zero jsx-a11y warnings in non-test files + lint ratchet CI gate#263njrini99-code wants to merge 1 commit into
Conversation
…atchet CI gate - 575+ jsx-a11y warnings fixed across src/app and src/components (label/control wiring via useId, clickable divs -> buttons, modal backdrops -> buttons, redundant role=list removed, justified disables for drag-drop/stopPropagation/tooltip/autofocus cases) - 0 jsx-a11y warnings remain outside 2 excluded test files - scripts/lint-ratchet.mjs: per-rule warning baseline (.lint-baseline.json, 2394 warnings/11 rules); 'npm run lint:ratchet' fails CI if any rule count rises; --update re-locks after intentional burn-down - ci.yml: ratchet step added after lint Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Too many files changed for review. ( |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedToo many files! This PR contains 174 files, which is 24 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (174)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21c7bcb6be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <button | ||
| type="button" | ||
| onClick={handleClear} |
There was a problem hiding this comment.
When a clearable Select already has a value, this renders the clear <button> inside the trigger <button> that starts at line 167. Nested buttons are invalid HTML and can cause Next.js hydration mismatches while also producing unreliable keyboard/focus behavior; MultiSelect repeats the same structure at line 422. Use a non-button trigger container or position the clear button as a sibling.
Useful? React with 👍 / 👎.
| <button | ||
| type="button" |
There was a problem hiding this comment.
On the mobile CRM view, this outer button contains a checkbox, favorite and menu buttons, status controls, and a mailto: link. Those nested interactive elements make the server-rendered markup invalid, which can trigger hydration recovery and prevents assistive technology from exposing the card controls reliably. Keep a non-interactive card container and provide a separate button or link for opening the coach.
Useful? React with 👍 / 👎.
| <button | ||
| type="button" |
There was a problem hiding this comment.
Whenever the invite modal opens, the backdrop button contains the entire dialog, including form controls and action buttons. Interactive descendants inside a button are invalid and give the dialog incorrect button semantics, leading to unreliable focus and keyboard activation. Render the backdrop as a sibling button or retain a non-interactive wrapper with a separate close control.
Useful? React with 👍 / 👎.
| <button | ||
| type="button" | ||
| aria-label="Close receipt viewer" | ||
| className="fixed inset-0 bg-warm-900/70 backdrop-blur-sm flex items-center justify-center z-50 p-4 w-full border-none cursor-default" | ||
| onClick={() => setViewingReceipt(null)} |
There was a problem hiding this comment.
When a receipt is viewed, this button wraps the modal's close IconButton and embedded receipt content. That creates invalid nested interactive content and causes the whole dialog to be exposed as a close button, which can break keyboard and screen-reader navigation. Make the backdrop a sibling of the modal rather than its interactive parent.
Useful? React with 👍 / 👎.
| <button | ||
| type="button" | ||
| onClick={handleClick} |
There was a problem hiding this comment.
When the compact card is rendered with showCheckbox, this new outer button contains the comparison Button at lines 105–117. The resulting nested buttons are invalid and can cause hydration or keyboard activation problems, including the card action competing with the comparison action. Keep the card container non-interactive and add a dedicated profile-opening control.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| </div> | ||
| )} | ||
| </div> | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
🔴 Nested buttons in CalendarView monthly grid: day-cell buttons contain event-pill buttons
The monthly calendar view converts day cells from <div> to <button> (line 260), but each day cell contains event pill <button> elements as children (line 284). The HTML spec states that <button> elements must not contain interactive content. Browsers handle nested buttons inconsistently — Chrome may forcibly close the outer button before the inner one, breaking layout; Firefox may make inner buttons unclickable. This affects all three calendar views (month, week, day) where the same pattern was applied.
Affected nesting chains
Month view (line 260→284): outer day <button> → inner event <button>
Week view (line 401): event <button> is absolutely positioned inside a <div> sibling to hour-slot <button>s — this one is fine since they're siblings.
Day view (line 503): same sibling pattern — also fine.
The month view is the actual problem because event pills are direct descendants of the day-cell button.
(Refers to lines 260-308)
Prompt for agents
In CalendarView.tsx monthly view, the day cell was converted from a div to a button (line 260), but it contains nested event-pill buttons (line 284) as children. Buttons cannot contain other buttons per HTML spec. The fix is to revert the outer day cell back to a div and instead add role="button", tabIndex={0}, and an onKeyDown handler (Enter/Space) to the day cell div. Alternatively, restructure so the clickable day area and the event pills are siblings rather than parent-child. The event pills at line 284 can remain as buttons since they use stopPropagation. The week view and day view don't have this problem since their event buttons are absolutely positioned siblings, not children of the hour-slot buttons.
Was this helpful? React with 👍 or 👎 to provide feedback.
| <IconChevronDown size={12} className="opacity-50" /> | ||
| </Button> | ||
| {isStatusOpen && ( | ||
| // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions -- stopPropagation-only wrapper prevents card click from closing status dropdown | ||
| <div className="absolute z-50 mt-1 py-1 min-w-[160px] max-h-[320px] overflow-y-auto bg-white/95 backdrop-blur-xl rounded-xl border border-warm-200/50 shadow-xl" onClick={e => e.stopPropagation()}> | ||
| {ALL_STATUSES.map(status => ( | ||
| <Button variant="primary" |
There was a problem hiding this comment.
🔴 Nested interactive elements: MobileCoachCard button wraps checkboxes, buttons, and dropdown menus
The MobileCoachCard component was converted from a <div> to a <button> at line 431, but it contains multiple nested interactive elements: <input type="checkbox"> (line 445-449), <IconButton> star toggle (line 462), <IconButton> more-options (line 471), status <Button> dropdown trigger (line 578), and dropdown <Button> items (line 594). While e.stopPropagation() wrappers exist to prevent event bubbling, the HTML is invalid — <button> cannot contain interactive content. This causes unpredictable behavior across browsers and breaks screen reader navigation of the card's internal controls.
(Refers to lines 431-641)
Prompt for agents
In CoachTable.tsx, the MobileCoachCard was converted from div to button at line 431, but it contains nested checkboxes, IconButtons, Buttons, and dropdown menus. Buttons cannot contain interactive content per HTML spec. Revert back to a div with role="button", tabIndex={0}, and an onKeyDown handler for Enter/Space. The existing onClick={handleCardClick} and stopPropagation wrappers on inner controls can remain. This preserves keyboard accessibility without creating invalid nested interactive elements.
Was this helpful? React with 👍 or 👎 to provide feedback.
| </Button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
🔴 Modal backdrop as wraps nested Button components (InvitePlayerButton)
The modal backdrop in InvitePlayerButton.tsx was changed from <div> to <button> (line 89), but the modal content inside contains multiple <Button> components: 'Try Again' (line 162-168), 'Copy Invite Link' (line 191-207), and 'Close' (line 232). This creates invalid nested interactive HTML. Unlike the BaseballInviteButton.tsx in the same PR which correctly uses role="presentation" on the backdrop <div>, the golf version wraps everything in a <button>.
(Refers to lines 89-237)
Prompt for agents
In InvitePlayerButton.tsx, the modal backdrop was changed to a <button> at line 89, but it wraps the entire modal dialog including other Button components. This creates nested interactive elements. Follow the pattern used by BaseballInviteButton.tsx in this same PR: use a <div> with role="presentation", add onKeyDown for Escape, and give the inner modal div role="dialog" with aria-modal="true". The backdrop click is already handled by the e.target === e.currentTarget check.
Was this helpful? React with 👍 or 👎 to provide feedback.
| )} | ||
| </div> | ||
| </motion.div> | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
🔴 Receipt viewer backdrop as wraps nested IconButton and iframe
In ExpenseList.tsx, the receipt viewer modal backdrop was converted from <div> to <button> (line 276), but it contains a motion.div child with an <IconButton> (line 290) and an <iframe> (line 299-302) — both interactive content. The <button> cannot contain these elements per HTML spec. This will cause the close IconButton and iframe to behave unpredictably across browsers.
(Refers to lines 276-313)
Prompt for agents
In ExpenseList.tsx line 276, the receipt viewer backdrop was changed to a <button> but wraps interactive children (IconButton at line 290 and iframe at line 299). Revert to a <div> and use role="presentation" or role="dialog" with an onKeyDown for Escape. The onClick handler at line 280 to close the viewer can stay on the div. Alternatively, separate the backdrop button and the modal content as siblings (the pattern used in MobileEventSheet.tsx in this same PR).
Was this helpful? React with 👍 or 👎 to provide feedback.
| <div className="text-warm-400"> | ||
| {isExpanded ? <IconChevronUp size={18} /> : <IconChevronDown size={18} />} | ||
| </div> | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
🟡 Expense row expand/collapse button wraps interactive child buttons
In ExpenseList.tsx, the expense row clickable area was changed from <div> to <button> at line 128-130, but when expanded, the AnimatePresence section rendered below it (starting at line 175) contains interactive <IconButton> elements for edit and delete actions. While the expanded content is technically a sibling in the motion wrapper, the outer <button> at line 128 does not self-close before the expanded region — the expanded section is rendered after the </button> at line 173. Let me re-verify... Actually, looking again: the <button> closes at line 173, and the AnimatePresence is a sibling. This is actually fine. Disregard.
(Refers to lines 128-173)
Was this helpful? React with 👍 or 👎 to provide feedback.
| <div | ||
| className="fixed inset-0 bg-warm-900/50 backdrop-blur-sm z-50 flex items-center justify-center p-4" | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-label="Close" | ||
| onClick={onClose} | ||
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') onClose(); }} |
There was a problem hiding this comment.
🚩 PlayerDetailModal uses role="button" on backdrop div containing interactive Card
The PlayerDetailModal.tsx backdrop <div> was given role="button" and tabIndex={0} at src/components/coach/PlayerDetailModal.tsx:65-66. While role="button" doesn't enforce HTML content model restrictions like a real <button> would, it still semantically declares the entire overlay (including the modal Card with close button, watchlist button, and message button) as a single button to assistive technology. Screen readers will announce the entire modal region as a button. The BaseballInviteButton.tsx pattern (role="presentation") would be more semantically correct for a backdrop overlay.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Folded into #304 (feat/coachhelm-stats-roundup) — merged clean, combined gates green. Branch intact + reopenable. |
1 participant
Summary
src/appandsrc/components(golf, fairway, coach, baseball, admin CRM, help) — 0 jsx-a11y warnings remain outside 2 excluded test files.useId+htmlFor/idwiring for form labels; clickable divs/rows and modal backdrops converted to<button type="button">; redundantrole="list"removed from<ul>; justified inline disables only for drag-and-drop zones, stopPropagation-only wrappers, hover-tooltip triggers, intentional dialog autofocus, and custom componentroleprops (not ARIA roles).scripts/lint-ratchet.mjs+.lint-baseline.jsonlock per-rule warning counts (2,394 warnings / 11 rules, down from 2,846 / 22).npm run lint:ratchetfails if any rule's count rises;--updatere-locks after intentional burn-down. Wired intoci.ymlafter the lint step.Test plan
npx tsc --noEmitcleannpx eslint src→ jsx-a11y only in the 2 excluded test files🤖 Generated with Claude Code