fix: make Hook Logs table rows keyboard-accessible#533
Conversation
📝 WalkthroughWalkthroughHook log table rows can now be focused and expanded with Enter or Space, expose their expanded state through ChangesHook log accessibility
Estimated code review effort: 2 (Simple) | ~5 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
|
Automated code review started - full review. Results will be posted here. |
![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
🧹 Nitpick comments (1)
app/components/session-hooks-panel.tsx (1)
331-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd keyboard interaction coverage.
Test focusing a row, pressing Enter and Space, and asserting that
aria-expandedand the detail panel update together.🤖 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 `@app/components/session-hooks-panel.tsx` around lines 331 - 334, Add keyboard interaction coverage for the row rendered by the session hooks panel: focus a row, press Enter and Space, and assert each key toggles both its aria-expanded value and the corresponding detail panel visibility together. Reuse the existing row/detail-panel test utilities and verify the collapsed and expanded states.
🤖 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 `@app/components/session-hooks-panel.tsx`:
- Around line 331-334: Update the row disclosure control around toggleRow so the
`<tr>` no longer carries role="button", keyboard handling, tabIndex, or
aria-expanded; place a native `<button>` with the same Enter/Space toggle
behavior inside a table cell, preserving the existing isExpanded state and
toggleRow(i) action.
---
Nitpick comments:
In `@app/components/session-hooks-panel.tsx`:
- Around line 331-334: Add keyboard interaction coverage for the row rendered by
the session hooks panel: focus a row, press Enter and Space, and assert each key
toggles both its aria-expanded value and the corresponding detail panel
visibility together. Reuse the existing row/detail-panel test utilities and
verify the collapsed and expanded states.
🪄 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: ae6aeed2-8fb0-406a-8e77-c9f23136cfd5
📒 Files selected for processing (1)
app/components/session-hooks-panel.tsx
| onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); toggleRow(i); } }} | ||
| tabIndex={0} | ||
| role="button" | ||
| aria-expanded={isExpanded} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant component around the changed lines
file="app/components/session-hooks-panel.tsx"
wc -l "$file"
sed -n '250,390p' "$file"Repository: FailproofAI/failproofai
Length of output: 7227
🌐 Web query:
WAI-ARIA role button on tr table row semantics aria-expanded treegrid table row accessibility
💡 Result:
Adding role="button" to a table row (tr element) is not recommended and negatively impacts accessibility [1][2][3]. Using this role on a tr element can remove the row from the accessibility tree in some screen readers, causing users to lose both the semantic information of the row and its functionality [2]. Regarding aria-expanded and treegrid semantics: 1. Treegrid Usage: The aria-expanded attribute is specifically valid for rows (tr) or their cells when they are part of a treegrid [4][5]. In a treegrid, this attribute indicates whether a parent row is expanded or collapsed to show or hide child rows [6][5]. 2. Standard Table/Grid Usage: Using aria-expanded on a tr element within a standard table or grid (role="table" or role="grid") is incorrect [4]. These attributes have no function in standard tables and can cause unpredictable behavior in assistive technologies [4]. 3. Accessibility Best Practices: - Avoid making entire table rows interactive by adding roles like "button" [7][2]. - Instead, place interactive controls (such as actual button or link elements) inside the cells of the row [7][2]. This approach ensures standard behavior, proper focus management, and better compatibility across different browsers and screen readers [2][8]. - If you need a tabular structure with expandable rows, implement the treegrid pattern correctly, ensuring the container has role="treegrid" and follow WAI-ARIA authoring practices for keyboard interaction and state management [4][6]. In summary, do not add role="button" to a tr, and reserve aria-expanded for genuine treegrid implementations where row expansion is natively supported by the design pattern [2][4].
Citations:
- 1: tr role="button" on table row mu-semtech/ember-data-table#20
- 2: https://www.tempertemper.net/blog/clickable-table-rows-are-a-bad-idea
- 3: https://www.w3.org/TR/2025/REC-html-aria-20250123/
- 4: https://dequeuniversity.com/rules/axe/4.12/aria-conditional-attr
- 5: https://github.com/mdn/content/blob/main/files/en-us/web/accessibility/aria/reference/attributes/aria-expanded/index.md
- 6: https://www.blackdogcat.xyz/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/treegrid_role
- 7: https://adrianroselli.com/2023/11/dont-turn-a-table-into-an-aria-grid-just-for-a-clickable-row.html
- 8: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/button_role
Move the disclosure control out of the <tr>
role="button" on the row can strip table semantics from the accessibility tree, and aria-expanded belongs on a real treegrid pattern. Put a <button> in a cell, or switch the whole table to treegrid.
🤖 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 `@app/components/session-hooks-panel.tsx` around lines 331 - 334, Update the
row disclosure control around toggleRow so the `<tr>` no longer carries
role="button", keyboard handling, tabIndex, or aria-expanded; place a native
`<button>` with the same Enter/Space toggle behavior inside a table cell,
preserving the existing isExpanded state and toggleRow(i) action.
|
✅ Build & test complete. Results:
|
| onClick={() => toggleRow(i)} | ||
| onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); toggleRow(i); } }} | ||
| tabIndex={0} | ||
| role="button" |
There was a problem hiding this comment.
🟡 ACCESSIBILITY CONCERN: role="button" on a tr element strips table semantics from the accessibility tree. Consider either converting the table to role="treegrid" or moving the button behavior to a native button inside a td cell.
| <React.Fragment key={`${item.timestamp}-${i}`}> | ||
| <tr | ||
| onClick={() => toggleRow(i)} | ||
| onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); toggleRow(i); } }} |
There was a problem hiding this comment.
🔵 INFO: No tests exist for SessionHooksPanel component. The new keyboard accessibility behavior (tabIndex, onKeyDown, aria-expanded) should ideally have test coverage:
- Test that Enter/Space toggles row expansion
- Test that aria-expanded reflects correct state
- Test that tabIndex makes rows focusable
Consider adding tests in tests/components/session-hooks-panel.test.tsx.
🔍 Automated Code Review📋 Executive SummaryThis PR adds keyboard accessibility to the Hook Logs table rows — making them focusable (tabIndex={0}), keyboard-operable (Enter/Space toggles), and announced by assistive technology (aria-expanded, role="button", aria-hidden on the chevron). The change is minimal (5 new lines) and directly addresses issue #525. Code quality is clean, the implementation follows established React patterns, and no breaking changes are introduced. One accessibility concern about role="button" on tr elements is noted as non-blocking. 📊 Change Architecturegraph TD
A["tr Row (existing)"] -->|"+onKeyDown handler"| B["Keyboard Accessible Row"]
A -->|"+tabIndex={0}"| B
A -->|"+role=button"| B
A -->|"+aria-expanded"| B
C["ChevronDown td"] -->|"+aria-hidden=true"| D["Decorative Icon"]
B --> E["isExpanded state"]
E --> F["toggleRow(i)"]
style B fill:#90EE90
style D fill:#87CEEB
Legend: 🟢 New | 🔵 Modified | 🟡 Breaking Change Risk 🔴 Breaking Changes✅ No breaking changes detected. Only additive changes to existing elements.
|
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
🔍 Automated Code Review
📋 Executive Summary
This PR adds keyboard accessibility to the Hook Logs table rows — making them focusable (tabIndex={0}), keyboard-operable (Enter/Space toggles), and announced by assistive technology (aria-expanded, role="button", aria-hidden on the chevron). The change is minimal (5 new lines) and directly addresses issue #525. Code quality is clean, the implementation follows established React patterns, and no breaking changes are introduced. One accessibility concern about role="button" on tr elements is noted as non-blocking.
📊 Change Architecture
graph TD
A["tr Row (existing)"] -->|"+onKeyDown handler"| B["Keyboard Accessible Row"]
A -->|"+tabIndex={0}"| B
A -->|"+role=button"| B
A -->|"+aria-expanded"| B
C["ChevronDown td"] -->|"+aria-hidden=true"| D["Decorative Icon"]
B --> E["isExpanded state"]
E --> F["toggleRow(i)"]
style B fill:#90EE90
style D fill:#87CEEB
Legend: 🟢 New | 🔵 Modified | 🟡 Breaking Change Risk
🔴 Breaking Changes
✅ No breaking changes detected. Only additive changes to existing elements.
⚠️ Issues Found
- 🟡 Accessibility —
app/components/session-hooks-panel.tsx:333—role="button"on a<tr>is discouraged by WAI-ARIA. See inline comment for alternatives (treegrid pattern or native button in td). - 🔵 Info —
__tests__/components/session-hooks-panel.test.tsx— No tests exist for this component. Keyboard behavior should be tested. See inline comment.
🔬 Logical / Bug Analysis
- onKeyDown handler: Correctly uses
e.key === "Enter" || e.key === " "' for Space (literal space character).e.preventDefault()` prevents browser scroll on Space. ✅ Correct. - aria-expanded: Correctly bound to
isExpandedstate. ✅ Correct. - aria-hidden on chevron: Prevents double announcement. ✅ Correct.
- No async/await or race conditions: The toggleRow handler is synchronous. ✅ Safe.
- No resource leaks: No timers, subscriptions, or connections to leak. ✅ Safe.
🧪 Evidence — Build & Test Results
Build Output
npm install: 252 packages, 0 vulnerabilities
tsc --noEmit: clean (no errors)
Test Results
Test Files 121 passed (121)
Tests 2066 passed (2066)
Duration 88.03s
🔗 Issue Linkage
This PR addresses #525 — "Dashboard Hook Logs table rows are not keyboard-accessible". The changes add keyboard focusability (tabIndex), Enter/Space toggling (onKeyDown), and screen reader announcement (role=button, aria-expanded, aria-hidden on chevron). All three requirements from the issue description are satisfied.
👥 Human Review Feedback
No human review comments on this PR. One CodeRabbit bot thread about role="button" accessibility — noted in inline comment, left open for maintainer decision.
💡 Suggestions
- Consider adding
data-testidattributes (data-testid="hook-row-expand") to make future test authoring easier. - The single-line ternary in onKeyDown is dense — extracting a
handleKeyDownfunction would improve readability if more keys are added in the future.
🏆 Verdict
VERDICT: APPROVED_WITH_SUGGESTIONS
Automated code review · 2026-07-16 12:27:00 UTC
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
|
Your PR is awaiting review by a moderator. Till then you can join the Discord for conversation: https://discord.gg/qaFM2uYFb |
|
Your PR is awaiting review by a moderator. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
|
Your PR is awaiting review by a reviewer. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
|
Tabbed through it — Enter/Space toggles and aria-expanded flips correctly, and hiding the chevron from AT is a nice touch.
|
3 participants
Closes #525
What this PR does:
Summary by CodeRabbit