Conversation
Elements with role="button" must handle both Enter and Space keys per WCAG 2.1 keyboard accessibility guidelines. The mobile video collapse toggle only responded to Enter, leaving keyboard-only users unable to activate it with Space. Also adds preventDefault() to avoid page scrolling on Space press. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||
📝 WalkthroughWalkthroughThe pull request enhances keyboard accessibility for the mobile video header by adding Space key support alongside the existing Enter key handling in the header toggle mechanism, enabling users to collapse or expand the video using either key. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/routes/song.$songId.tsx (2)
671-671: 🧹 Nitpick | 🔵 TrivialInconsistent Space key detection:
event.code === 'Space'(line 671) vse.key === ' '(line 706).Both detect the Space key on standard keyboards, but mixing
code(physical key position) andkey(character value) within the same file is inconsistent. Usinge.key === ' '(as in the new handler) is the more semantically correct choice for character-intent checks; consider aligning the global handler at line 671 to match.♻️ Align global handler to use `e.key`
- if (event.code === 'Space' && event.target === document.body) { + if (event.key === ' ' && event.target === document.body) {Also applies to: 706-706
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/song`.$songId.tsx at line 671, The global key handler currently checks the Space key using event.code === 'Space' while another handler uses e.key === ' ' — make them consistent by switching the global check to use event.key === ' ' (and keep the existing document.body target check) so both handlers detect character-intent Space uniformly; update the conditional in the global handler (the function that contains event.code === 'Space') to use event.key === ' ' and verify associated logic still behaves as expected.
701-711:⚠️ Potential issue | 🟡 Minor
role="button"is missingaria-expanded— screen readers cannot convey collapsed/expanded state.A
role="button"that toggles visibility should exposearia-expandedper WCAG 2.1 SC 4.1.2. Without it, AT users pressing the button have no programmatic feedback about whether the video is currently collapsed or expanded.♿ Proposed fix
<div role="button" tabIndex={0} + aria-expanded={!isVideoCollapsed} + aria-label={isVideoCollapsed ? "Expand video" : "Collapse video"} onClick={handleVideoCollapsedToggle}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/song`.$songId.tsx around lines 701 - 711, The button-like div with role="button" that calls handleVideoCollapsedToggle must expose its current expanded state via aria-expanded; add an aria-expanded attribute bound to the boolean state that tracks whether the video is collapsed/expanded (the same state used by handleVideoCollapsedToggle, e.g., isVideoCollapsed or videoCollapsed) on the element so screen readers can announce the collapsed/expanded state and keep role="button" and keyboard handlers unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/routes/song.$songId.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TanStack Start framework with Bun runtime for the application
Tests must pass locally via
bun run testbefore committing code, as Husky pre-commit hooks will block commits with failing tests
Files:
src/routes/song.$songId.tsx
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Components with logic MUST have corresponding test files following the naming convention
ComponentName.test.tsx
Files:
src/routes/song.$songId.tsx
🔇 Additional comments (1)
src/routes/song.$songId.tsx (1)
705-710:⚠️ Potential issue | 🟡 MinorAdd behavioral tests for the new keyboard interaction logic to
src/__tests__/songPageDefaults.test.ts.While a test file exists for the song page, it only performs static file-content checks. The new
onKeyDownhandler with Space and Enter key support lacks behavioral test coverage for: (a) Space key toggling the collapse, (b) Enter key toggling the collapse, (c)preventDefaultpreventing default scroll behavior, (d) unrelated keys being ignored. Extend the existing integration test to cover these keyboard interaction scenarios.⛔ Skipped due to learnings
Learnt from: CR Repo: EtanHey/songscript PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-28T12:17:53.877Z Learning: Applies to **/*.tsx : Components with logic MUST have corresponding test files following the naming convention `ComponentName.test.tsx`Learnt from: CR Repo: EtanHey/songscript PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-28T12:17:53.877Z Learning: Applies to **/*.ts : All new helpers and utilities MUST have corresponding test filesLearnt from: CR Repo: EtanHey/songscript PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-28T12:17:53.877Z Learning: Applies to **/*.test.tsx : Component tests should be located in the same directory as the component file with naming convention `ComponentName.test.tsx`
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/song`.$songId.tsx:
- Around line 705-710: The keyboard handler on the role="button" wrapper is
missing a guard and aria state: update the onKeyDown handler (the arrow function
that calls handleVideoCollapsedToggle) to first check that e.target ===
e.currentTarget before calling e.preventDefault() or handleVideoCollapsedToggle
so events from nested interactive elements like ToggleGroupItem or
WishlistButton don't bubble and trigger the toggle; also add aria-expanded on
the role="button" div and bind it to the component's collapsed state (the same
state used by handleVideoCollapsedToggle) so screen readers know the
expanded/collapsed status; finally add a corresponding test file
(song.$songId.test.tsx) to cover keyboard interaction and nesting cases.
---
Outside diff comments:
In `@src/routes/song`.$songId.tsx:
- Line 671: The global key handler currently checks the Space key using
event.code === 'Space' while another handler uses e.key === ' ' — make them
consistent by switching the global check to use event.key === ' ' (and keep the
existing document.body target check) so both handlers detect character-intent
Space uniformly; update the conditional in the global handler (the function that
contains event.code === 'Space') to use event.key === ' ' and verify associated
logic still behaves as expected.
- Around line 701-711: The button-like div with role="button" that calls
handleVideoCollapsedToggle must expose its current expanded state via
aria-expanded; add an aria-expanded attribute bound to the boolean state that
tracks whether the video is collapsed/expanded (the same state used by
handleVideoCollapsedToggle, e.g., isVideoCollapsed or videoCollapsed) on the
element so screen readers can announce the collapsed/expanded state and keep
role="button" and keyboard handlers unchanged.
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault(); | ||
| handleVideoCollapsedToggle(); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
wc -l src/routes/song.$songId.tsxRepository: EtanHey/songscript
Length of output: 113
🏁 Script executed:
# Read the onKeyDown handler and surrounding context (lines 695-750)
sed -n '695,750p' src/routes/song.$songId.tsx | cat -nRepository: EtanHey/songscript
Length of output: 125
🏁 Script executed:
# Check for test file for this component
find . -name "*song*test*" -o -name "*test*song*" | head -20Repository: EtanHey/songscript
Length of output: 102
🏁 Script executed:
# Search for stopPropagation in the tsx files to understand propagation handling
rg "stopPropagation" --type tsx -B2 -A2 | head -100Repository: EtanHey/songscript
Length of output: 89
🏁 Script executed:
# Check the global Space handler implementation (around lines 669-679)
sed -n '665,685p' src/routes/song.$songId.tsx | cat -nRepository: EtanHey/songscript
Length of output: 125
🏁 Script executed:
# Find the actual song route file
find . -path "*/routes/*song*" -type f | head -20Repository: EtanHey/songscript
Length of output: 91
🏁 Script executed:
# List route files to see naming pattern
ls -la src/routes/ | head -20Repository: EtanHey/songscript
Length of output: 724
🏁 Script executed:
# Read the file with proper escaping - use single quotes to prevent variable expansion
wc -l './src/routes/song.$songId.tsx'Repository: EtanHey/songscript
Length of output: 95
🏁 Script executed:
# Read the onKeyDown handler and surrounding context (lines 695-750)
sed -n '695,750p' './src/routes/song.$songId.tsx' | cat -nRepository: EtanHey/songscript
Length of output: 3641
🏁 Script executed:
# Check the global Space handler implementation (around lines 665-685)
sed -n '665,685p' './src/routes/song.$songId.tsx' | cat -nRepository: EtanHey/songscript
Length of output: 887
🏁 Script executed:
# Search for aria-expanded in the file
rg "aria-expanded" './src/routes/song.$songId.tsx' -nRepository: EtanHey/songscript
Length of output: 44
🏁 Script executed:
# Check wrapper divs at lines 719-746 and their stopPropagation calls
sed -n '700,746p' './src/routes/song.$songId.tsx' | cat -nRepository: EtanHey/songscript
Length of output: 3158
🏁 Script executed:
# Check for test file for song.$songId.tsx with different naming patterns
find . -name "*song*test*" -o -name "*test*song*" -o -path "*__tests__*song*" 2>/dev/nullRepository: EtanHey/songscript
Length of output: 102
🏁 Script executed:
# Check if there's a test file matching the component filename exactly
ls -la './src/routes/song.$songId.test.tsx' 2>&1Repository: EtanHey/songscript
Length of output: 143
🏁 Script executed:
# Search for any onKeyDown stopPropagation in the codebase to see if it's a pattern
rg "onKeyDown" --type tsx -A3 | grep -A3 "stopPropagation"Repository: EtanHey/songscript
Length of output: 89
🏁 Script executed:
# Verify ToggleGroup and WishlistButton implementations - check if they handle keydown internally
rg "WishlistButton\|ToggleGroup" './src/routes/song.$songId.tsx' -B2 -A5Repository: EtanHey/songscript
Length of output: 44
🏁 Script executed:
# Check what library provides ToggleGroup to understand keydown behavior
rg "from.*ToggleGroup\|import.*ToggleGroup" --type tsxRepository: EtanHey/songscript
Length of output: 89
🏁 Script executed:
# Search for ToggleGroup import
grep -n "ToggleGroup" './src/routes/song.$songId.tsx' | head -10Repository: EtanHey/songscript
Length of output: 928
🏁 Script executed:
# Search for imports at the top of the file
head -50 './src/routes/song.$songId.tsx' | cat -nRepository: EtanHey/songscript
Length of output: 2532
🏁 Script executed:
# Check if WishlistButton or ToggleGroup stop keydown propagation by searching their source files
find ./src -name "WishlistButton*" -o -name "*ToggleGroup*" -o -name "*wishlist*" | head -20Repository: EtanHey/songscript
Length of output: 97
🏁 Script executed:
# Look for test files in the codebase to understand the pattern
find ./src -name "*.test.ts" -o -name "*.test.tsx" | head -20Repository: EtanHey/songscript
Length of output: 457
🏁 Script executed:
# Check the ToggleGroup implementation to see if it stops keydown propagation
cat -n './src/components/ui/toggle-group.tsx'Repository: EtanHey/songscript
Length of output: 2956
🏁 Script executed:
# Check WishlistButton to see if it handles keydown
cat -n './src/components/WishlistButton.tsx'Repository: EtanHey/songscript
Length of output: 6003
🏁 Script executed:
# Verify the exact line numbers in the original file
grep -n "onKeyDown" './src/routes/song.$songId.tsx' | head -5Repository: EtanHey/songscript
Length of output: 101
🏁 Script executed:
# Check if this component file should have a test based on coding guidelines
# Verify the test naming convention used in existing test files
ls -la './src/components/'*.test.tsx | head -10Repository: EtanHey/songscript
Length of output: 330
🏁 Script executed:
# Verify the behavior of ToggleGroupItem keydown by checking Radix UI behavior
# See if it implements any keydown handling that might stop propagation
rg "ToggleGroupItem\|ToggleGroupPrimitive" './src/components/ui/toggle-group.tsx' -A10 | head -40Repository: EtanHey/songscript
Length of output: 44
🏁 Script executed:
# Check if there's any keydown stopPropagation in the entire route file
grep -n "stopPropagation" './src/routes/song.$songId.tsx'Repository: EtanHey/songscript
Length of output: 234
🏁 Script executed:
# Check if the Radix UI ToggleGroupItem handles keydown internally and whether it stops propagation
# Look at how Radix UI typically handles Space/Enter on toggle buttons
web_search "Radix UI ToggleGroup keydown event propagation space enter"Repository: EtanHey/songscript
Length of output: 112
🏁 Script executed:
# Verify aria-expanded is indeed missing from the role="button" div
grep -n "aria-expanded" './src/routes/song.$songId.tsx'Repository: EtanHey/songscript
Length of output: 44
🏁 Script executed:
# Confirm coding guideline: check if route page components are typically tested
find ./src/routes -name "*.test.tsx" -o -name "*.test.ts"Repository: EtanHey/songscript
Length of output: 44
🏁 Script executed:
# Double-check the exact content and structure around the onKeyDown to confirm line numbers in review
sed -n '700,720p' './src/routes/song.$songId.tsx' | cat -nRepository: EtanHey/songscript
Length of output: 1316
🏁 Script executed:
# Verify if Radix UI ToggleGroupItem has built-in Space/Enter handling by checking if it prevents default
grep -A20 "ToggleGroupPrimitive.Item" './src/components/ui/toggle-group.tsx' | head -25Repository: EtanHey/songscript
Length of output: 986
Add guard to prevent bubbling from nested interactive elements and missing aria attributes.
The wrapper divs at lines 719 and 725 only call e.stopPropagation() on onClick, not on onKeyDown. When a keyboard user focuses a ToggleGroupItem or WishlistButton and presses Space/Enter, the keydown event bubbles to this handler, which:
- Calls
handleVideoCollapsedToggle()unintentionally - Calls
e.preventDefault()on a bubbled Space event — on some browsers this suppresses theclickevent that the nested<button>fires on keyup, potentially breaking the nested control
Additionally, the div[role="button"] is missing the aria-expanded attribute to convey its collapsed/expanded state to screen readers.
Add a target === currentTarget guard and add aria-expanded:
🛡️ Proposed fix
<div
role="button"
tabIndex={0}
+ aria-expanded={!isVideoCollapsed}
onClick={handleVideoCollapsedToggle}
onKeyDown={(e) => {
- if (e.key === 'Enter' || e.key === ' ') {
+ if ((e.key === 'Enter' || e.key === ' ') && e.target === e.currentTarget) {
e.preventDefault();
handleVideoCollapsedToggle();
}
}}Per coding guidelines, components with logic must have corresponding test files. Add src/routes/song.$songId.test.tsx.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/song`.$songId.tsx around lines 705 - 710, The keyboard handler on
the role="button" wrapper is missing a guard and aria state: update the
onKeyDown handler (the arrow function that calls handleVideoCollapsedToggle) to
first check that e.target === e.currentTarget before calling e.preventDefault()
or handleVideoCollapsedToggle so events from nested interactive elements like
ToggleGroupItem or WishlistButton don't bubble and trigger the toggle; also add
aria-expanded on the role="button" div and bind it to the component's collapsed
state (the same state used by handleVideoCollapsedToggle) so screen readers know
the expanded/collapsed status; finally add a corresponding test file
(song.$songId.test.tsx) to cover keyboard interaction and nesting cases.
1 participant
User description
Automated improvement by Golems Night Shift.
fix a11y Space key video toggle
PR Type
Bug fix, Enhancement
Description
Add Space key support to video toggle button for WCAG compliance
Prevent default page scroll behavior when Space key is pressed
Improve keyboard accessibility for mobile video collapse feature
Diagram Walkthrough
File Walkthrough
song.$songId.tsx
Add Space key handling to video toggle buttonsrc/routes/song.$songId.tsx
onKeyDownhandler to accept both Enter and Space keyse.preventDefault()to prevent page scrolling on Space pressNote
Low Risk
Small, localized UI a11y change limited to a keyboard event handler; minimal behavioral risk beyond key interaction.
Overview
Improves keyboard accessibility for the mobile video section’s collapsible header in
src/routes/song.$songId.tsx.The header’s
onKeyDownhandler now toggles on both Enter and Space, and callspreventDefault()for Space to avoid page scrolling while activating the collapse/expand behavior.Written by Cursor Bugbot for commit 7281d75. This will update automatically on new commits. Configure here.
Summary by CodeRabbit