fix: make Hook Logs table rows keyboard-accessible#575
Conversation
Signed-off-by: Abhishek Hiremath <131762197+Abhi-DevHub@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughHook log expansion controls in both panel implementations are now focusable buttons with keyboard support, ARIA state labels, event isolation, and preserved chevron rotation. Tests verify initial, Space, Enter, and click interactions. ChangesHook log accessibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 |
|
Your PR is awaiting review by a reviewer. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
![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
🤖 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/policies/hooks-client.tsx`:
- Around line 690-702: Update the ChevronDown element in the hook event toggle
button to include aria-hidden="true", relying on the existing button aria-label
for screen-reader context.
🪄 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: 3493a1cf-5396-421a-a512-6d07e2f9ac6c
📒 Files selected for processing (3)
__tests__/components/session-hooks-panel.test.tsxapp/components/session-hooks-panel.tsxapp/policies/hooks-client.tsx
|
Automated code review started - full review. Results will be posted here. |
chhhee10
left a comment
chhhee10
left a comment
There was a problem hiding this comment.
This is exactly it — approving.
You hit all three points, and the one I hadn't spelled out: e.stopPropagation() on the button's click. Without it the button handler and the row's onClick both fire, the row toggles twice, and it looks like the button does nothing. That's the trap this shape sets and you walked around it.
The semantics are right now. The <tr> carries only onClick — no role, no tabIndex, no aria-expanded — so the row and cell roles are intact and a screen reader can still move through the columns. The toggle is a real button with aria-expanded, a label that flips between Collapse/Expand, and a focus-visible ring so keyboard users can see where they are. Fixing hooks-client.tsx in the same pass was the right call too.
What I ran
Merged onto main: tsc clean, lint 0 errors, build clean, 2237 tests passed (the one failure is the pre-existing Pi integration > writeHookEntries from #569, which asserts the checkout directory is named failproofai — it fails the same way on untouched main).
I also checked the tests actually bite, by reverting the component to main and re-running: all four fail with Unable to find an accessible element with the role "button" and name /expand hook event details/i. They'd fail against the previous role="button"-on-the-row version too, since that had no accessible name. That's the good kind of test — it fails for the reason it exists.
And this merges cleanly alongside the four approved PRs (#527/#528/#529/#531); I merged all five together and the suite stayed green, so merge order doesn't matter.
Two nits — not blocking, take them or leave them
1. aria-hidden is inconsistent between the two files. The chevron in session-hooks-panel.tsx has aria-hidden="true"; the one in hooks-client.tsx doesn't. No practical difference — the button's aria-label overrides the accessible name either way — but the two blocks are otherwise identical and it'd be nice if they stayed that way.
2. The lucide mock is one fixture change away from a confusing failure. vi.mock("lucide-react", …) stubs 7 icons, but PaginationControls imports ChevronLeft and ChevronRight. It passes today only because PaginationControls returns null at totalPages <= 1 and the fixture uses 1. The moment someone sets totalPages: 2 to test pagination, those render as undefined and you get "Element type is invalid", which is a rough thing to debug. Adding the two icons to the mock now costs a line and saves that.
Thanks for coming back to this after the first attempt got closed — the second version is a genuinely better fix than the shape I sketched, and the sibling table means nobody has to come back for it.
|
One-liner and this is good to merge — CodeRabbit landed on the same
<ChevronDown
className={`h-3.5 w-3.5 text-muted-foreground transition-transform duration-150 ${
isExpanded ? "rotate-0" : "-rotate-90"
}`}
aria-hidden="true"
/>If you're pushing anyway, the lucide mock nit from my review is worth folding into the same commit — adding Approval's already on the PR and CI is green across all 11 checks, so once that's pushed and the thread resolves it can go straight in. |
🔍 Automated Code Review — PR #575📋 Executive SummaryThis PR fixes keyboard accessibility for expandable hook-log table rows by replacing inline Overall quality: Good. One minor inconsistency found where 📊 Change Architecturegraph TD
A["<tr> — mouse-only onClick"] --> B["New: <button> — keyboard handler"]
B --> C["aria-expanded"]
B --> D["aria-label"]
B --> E["focus-visible ring"]
B --> F["ChevronDown — aria-hidden"]
A --> G["Tests: Tab / Enter / Space"]
style B fill:#90EE90
style G fill:#90EE90
Legend: 🟢 New | 🔵 Modified 🔴 Breaking Changes✅ No breaking changes detected. This is a purely additive accessibility improvement:
|
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: CHANGES_REQUESTED 🔴
Found 1 blocking issue: hooks-client.tsx:701 is missing aria-hidden="true" on the ChevronDown icon — the identical pattern in session-hooks-panel.tsx already has it. One-line fix needed.
All 2068 tests pass, TypeScript/lint clean. See summary comment for full details.
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
CodeRabbit + maintainer review: add aria-hidden='true' to ChevronDown in hooks-client.tsx (matching session-hooks-panel.tsx), and add ChevronLeft/ChevronRight to lucide-react mock (PaginationControls imports them, test only survives today because it returns null).
0c18298
|
Automated code review started - full review. Results will be posted here. |
|
✅ Build \u0026 test complete. Results:
|
Automated Code Review - PR #575Executive SummaryThis PR adds keyboard accessibility to expandable hook-log table rows across both Change Architecturegraph TD
A["tr onClick"] -->|"kept as mouse fallback"| B["td chevron cell"]
B --> C["button with aria-expanded"]
C --> D["ChevronDown aria-hidden=true"]
D --> E["toggleRow(i) — accordion single-row expand"]
F["New tests"] --> G["4 vitest cases"]
G --> H["Enter toggle"]
G --> I["Space toggle"]
G --> J["Click toggle"]
G --> K["aria-expanded verification"]
style C fill:#90EE90
style F fill:#90EE90
style A fill:#87CEEB
Legend: Green = New, Blue = Modified Breaking ChangesNo breaking changes detected. The Issues FoundNo blocking issues. Minor observations:
Logical / Bug Analysis
Evidence - Build and Test ResultsTest Results - 2068/2068 passed: Other Checks:
Issue LinkageCloses #525 - the original issue requested keyboard accessibility for hook log rows. This PR delivers exactly that with a native Human Review Feedback (from Phase 0)
Suggestions
VerdictVERDICT: APPROVED Automated code review · 2026-07-21 05:38 UTC |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. All checks pass, aria-hidden fix applied, 4 new tests validate keyboard behavior. Clean, low-risk accessibility improvement. ✅
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
3 participants
Closes #525
What this PR does
Checklist
Summary by CodeRabbit
Bug Fixes
Accessibility
aria-expanded, descriptive labels, and decorative chevron markup.Tests