fix: add keyboard navigation support #2737
Conversation
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds keyboard handlers and focus-visible styling to multiple interactive frontend components, extends ActionButton to accept onKeyDown and className, introduces a dev page showcasing ModuleList scenarios, and adds ARIA/label improvements for accessibility. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/ModuleList.tsx (1)
23-31: Consider removing button semantics from non-interactive module badges.The module badges are rendered as
<button>elements but lack any click or keyboard handlers, making them appear interactive when they're actually static display elements. This creates a confusing experience for keyboard users who can tab to these elements but cannot activate them.Consider one of these solutions:
Option 1 (Recommended): Use a
<span>instead if these are purely presentational:- <button + <span key={`${module}-${index}`} - className="rounded-lg border border-gray-400 px-3 py-1 text-sm transition-all duration-200 ease-in-out hover:scale-105 hover:bg-gray-200 dark:border-gray-300 dark:hover:bg-gray-700 focus-visible:ring-1" + className="inline-block rounded-lg border border-gray-400 px-3 py-1 text-sm" title={module.length > 50 ? module : undefined} - type="button" > {displayText} - </button> + </span>Option 2: If these should be interactive, add onClick/onKeyDown handlers (e.g., to filter or search by module).
🧹 Nitpick comments (2)
frontend/src/components/Search.tsx (2)
79-88: Accessible name: consider simplifying label vsaria-label, and watch for staticidreuseYou now have both an associated
<label>and anaria-labelon the same input; screen readers will usearia-labelas the accessible name and effectively ignore the label. Since you already provide a label (even if sr-only), you can simplify by dropping thearia-labeland let the label + placeholder carry the meaning. This also avoids coupling the accessible name to whatever string is passed asplaceholder.Additionally, if
SearchBarcan be rendered more than once on a page, a hard‑codedid="search-input"will produce duplicate IDs; consider parameterizing the id or generating a unique one per instance.Example minimal change:
- aria-label={placeholder || 'Search'}Please verify with a screen reader (e.g., navigating to the field and listening to its announced name) and confirm that
SearchBaris either singleton or given unique IDs where reused. Based on learnings, keeping ARIA semantics simple and unambiguous improves overall accessibility.
92-97:Clear searchbutton: focus-visible is good; addtype="button"and rely on native keyboard supportThe added
aria-label="Clear search"andfocus-visiblering classes look good and satisfy the focus indication requirement. Since this is a real<button>, it is already tabbable and responds to Enter/Space by default, so there’s no need for an extraonKeyDownhandler (which could risk double activation).To avoid accidental form submissions if
SearchBaris ever placed inside a<form>, it’s safer to declare the button type explicitly:- {searchQuery && ( - <button - aria-label="Clear search" + {searchQuery && ( + <button + type="button" + aria-label="Clear search" className="absolute top-1/2 right-2 -translate-y-1/2 rounded-full p-1 hover:bg-gray-100 focus:outline-none focus-visible:ring-2 focus-visible:ring-gray-300 focus-visible:ring-offset-1" onClick={handleClearSearch} >Please verify by tabbing to the clear button and pressing Enter/Space to confirm it activates once and does not submit any enclosing form. Based on learnings, consistent keyboard and ARIA patterns across interactive controls improve overall accessibility.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/src/components/ActionButton.tsx(2 hunks)frontend/src/components/Card.tsx(1 hunks)frontend/src/components/ModuleList.tsx(2 hunks)frontend/src/components/ProgramActions.tsx(2 hunks)frontend/src/components/Release.tsx(1 hunks)frontend/src/components/Search.tsx(1 hunks)frontend/src/components/ShowMoreButton.tsx(1 hunks)frontend/src/components/SortBy.tsx(3 hunks)frontend/src/components/ToggleableList.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/components/ModuleList.tsxfrontend/src/components/ActionButton.tsxfrontend/src/components/SortBy.tsxfrontend/src/components/Card.tsx
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/ShowMoreButton.tsxfrontend/src/components/ProgramActions.tsxfrontend/src/components/Release.tsxfrontend/src/components/ToggleableList.tsx
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/src/components/ActionButton.tsxfrontend/src/components/Card.tsx
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/ProgramActions.tsx
[warning] 134-138: Elements with the 'menu' interactive role must be focusable.
🔇 Additional comments (8)
frontend/src/components/Release.tsx (1)
72-94: LGTM! Proper keyboard accessibility implementation.The button correctly implements:
- Explicit
type="button"attribute- Descriptive
aria-labelfor screen readers- Keyboard activation via Enter and Space keys with proper
preventDefault()to avoid page scroll- Visible focus indicator using
focus-visible:ring-2frontend/src/components/SortBy.tsx (1)
63-87: Excellent accessibility implementation!The sort order toggle button demonstrates best practices:
aria-labelprovides clear purposearia-pressedcorrectly reflects toggle state- Keyboard activation via Enter and Space
- Comprehensive focus-visible styling
- Maintains existing onClick behavior
frontend/src/components/Card.tsx (1)
146-161: LGTM! Proper integration with enhanced ActionButton API.The ActionButton usage correctly leverages the new keyboard accessibility features:
onKeyDownhandler mirrors theonClickbehavior- Focus-visible styling via
className- Proper event handling with
preventDefault()for Space keyfrontend/src/components/ProgramActions.tsx (3)
68-74: Good focus management on menu open.The useEffect correctly focuses the first menu item when the dropdown opens, providing a smooth keyboard navigation experience. Using
requestAnimationFrameensures the DOM is ready before attempting to focus.
76-113: Excellent keyboard navigation implementation!The menu keyboard handler demonstrates best practices:
- Arrow keys for cyclical navigation
- Home/End for first/last item
- Escape to close and restore focus
- Proper
preventDefault()to avoid scroll behaviorThis aligns well with the accessibility patterns from learnings.
117-152: SonarCloud warning is a false positive.The static analyzer warns that the
<div role="menu">must be focusable, but this is not correct for the ARIA menu pattern being used here. The implementation is correct:
- The trigger button (line 117) is focusable and has proper ARIA attributes (
aria-haspopup,aria-expanded)- Individual menu items (line 140-148) are focusable buttons with
role="menuitem"- The menu container serves as a semantic grouping element, not a focusable element itself
According to ARIA best practices, in a menu pattern, the individual menu items are focusable, not the menu container. The current implementation correctly follows this pattern.
Based on learnings, this properly implements dropdown menu accessibility with ARIA attributes, keyboard navigation (Enter, Space, Escape, arrow keys), and mechanisms to close when clicking outside.
frontend/src/components/ActionButton.tsx (2)
6-13: Clean API extension for accessibility features.The interface correctly extends ActionButton to support:
onKeyDownwith proper typing for both Link and Button element typesclassNamefor custom focus stylingBoth props are optional, maintaining backward compatibility.
15-41: Proper propagation of keyboard and styling props.The implementation correctly:
- Passes
onKeyDownto both Link and Button variants- Composes
classNamewith existingbaseStylesusing template strings- Maintains consistent behavior across both rendering paths
There was a problem hiding this comment.
Please address CodeRabbit comments and resolve conflicts before we can review.
Also, one new issue was introduced in this PR - https://sonarcloud.io/project/issues?id=OWASP_Nest&pullRequest=2737&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true
Thanks!
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)
frontend/src/components/ProgramActions.tsx (1)
140-150: Add tabindex="-1" to menu items for proper ARIA menu pattern.In the ARIA menu pattern, menu items should have
tabindex="-1"so focus is managed programmatically (via arrow keys) rather than through the natural tab order. Without this, keyboard users can tab through items, which conflicts with the arrow key navigation and breaks the expected menu behavior.Apply this diff:
<button key={option.key} type="button" role="menuitem" + tabIndex={-1} onClick={() => handleAction(option.key)} className="block w-full px-4 py-2 text-left text-sm hover:bg-gray-100 dark:hover:bg-gray-700 focus:outline-none focus-visible:ring-1 focus-visible:ring-offset-1" >
🧹 Nitpick comments (2)
frontend/src/components/Search.tsx (1)
79-83: Accessible label and input id are good; consider making the id configurable.The
sr-onlylabel andhtmlFor="search-input"/id="search-input"pairing correctly expose the search field to assistive tech. IfSearchBarcan appear multiple times on a page, a hard-codedidmay cause duplicate IDs; consider taking an optionalidprop or deriving a unique id when needed.frontend/src/components/ProgramActions.tsx (1)
123-128: Optional: Redundant keyboard handler on trigger button.Native
<button>elements automatically fireonClickwhen Enter or Space is pressed. TheonKeyDownhandler duplicates this behavior. While not incorrect, it adds unnecessary code.Consider removing the
onKeyDownhandler to simplify:onClick={() => setDropdownOpen((prev) => !prev)} - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault() - setDropdownOpen((prev) => !prev) - } - }} className="rounded px-4 py-2 hover:bg-gray-200 dark:hover:bg-gray-700 focus:outline-none focus-visible:ring-1 focus-visible:ring-offset-1"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/ProgramActions.tsx(2 hunks)frontend/src/components/Search.tsx(2 hunks)frontend/src/components/ShowMoreButton.tsx(1 hunks)frontend/src/components/ToggleableList.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/ProgramActions.tsxfrontend/src/components/ShowMoreButton.tsxfrontend/src/components/ToggleableList.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/components/ShowMoreButton.tsxfrontend/src/components/ToggleableList.tsx
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/src/components/ShowMoreButton.tsxfrontend/src/components/ToggleableList.tsx
🔇 Additional comments (10)
frontend/src/components/Search.tsx (1)
90-97: Clear button semantics and focus-visible styling align with a11y goals.
type="button"prevents accidental form submission,aria-label="Clear search"gives the icon-only control an accessible name, and thefocus-visiblering classes provide a clear keyboard focus indicator while keeping mouse focus unobtrusive. This meets the keyboard and focus requirements well.frontend/src/components/ShowMoreButton.tsx (3)
20-21: ARIA attributes correctly implement toggle semantics.The
aria-expandedattribute properly reflects the toggle state, and the dynamicaria-labelprovides clear context for screen readers. This follows WCAG 2.1 best practices for interactive controls.
22-22: Focus styling correctly implements keyboard focus indicator.The
focus-visible:ring-1andfocus-visible:ring-offset-1classes properly provide a visible focus indicator for keyboard navigation while avoiding mouse-click focus rings. This aligns with WCAG 2.1 requirements.
16-23: Based on the web search results, I can now provide verification. The web search definitively confirms that HeroUI's Button component automatically handles Enter and Space keypresses through theonPressprop, which is exactly what the ShowMoreButton is using.Since the repository cloning is unavailable in the sandbox environment, I'll work with the verification result from the web search and the information provided in the review comment. The web search evidence is authoritative and directly answers the original concern.
HeroUI Button automatically handles Enter and Space keyboard activation through the onPress prop.
The current implementation using
onPress={handleToggle}is sufficient for keyboard accessibility. HeroUI normalizes keyboard (Enter/Space), mouse, and touch interactions, so no explicitonKeyDownhandler is required for standard button activation. The component also includes proper ARIA attributes (aria-expanded,aria-label) which enhance accessibility.The focus ring styling (
focus-visible:ring-1 focus-visible:ring-offset-1) ensures keyboard navigation visibility.Note: If the toggle behavior needs to close on Escape key press (per accessibility best practices for interactive components), that would require explicit handling via
onKeyDownfor the Escape key, but that's beyond standard button activation.frontend/src/components/ProgramActions.tsx (4)
68-74: Excellent focus management on menu open.Using
requestAnimationFrameensures the DOM is ready before focusing the first menu item. This follows ARIA menu pattern best practices.
76-113: Comprehensive keyboard navigation implementation.The arrow key navigation, Home/End shortcuts, and Escape handling correctly implement the ARIA menu pattern. The wrapping behavior on ArrowUp/Down provides excellent UX.
120-121: Well-implemented ARIA attributes and focus styling.The combination of
aria-haspopup="menu",aria-expanded, andfocus-visible:ring-1provides excellent accessibility. Screen readers understand the button's purpose, and keyboard users get clear visual feedback.Based on learnings, this aligns with accessibility best practices for dropdown menus.
Also applies to: 129-129
146-146: Clear focus indicators for menu items.The
focus-visiblestyling provides keyboard users with clear visual feedback, meeting WCAG 2.1 requirements for visible focus indicators.frontend/src/components/ToggleableList.tsx (2)
42-45: Button attributes and focus-visible styling look good.Explicit
type="button", the descriptivearia-label, and thefocus-visiblering classes together give proper semantics and a visible keyboard focus indicator, matching the PR’s a11y goals.Please quickly sanity-check with keyboard-only navigation (Tab / Shift+Tab + Enter/Space) in a real browser to ensure the focus ring contrast and label text feel right in context.
45-51: Verify keyboard activation behavior on the native<button>element to confirm potential double-firing.The underlying accessibility concern is valid: native
<button>elements already handle Enter and Space keys natively, and adding a customonKeyDownhandler that calls the same action can indeed cause double-activation. This has been confirmed as a known pattern in accessibility best practices.However, I was unable to access the repository directly to verify:
- Whether this is actually a native
<button>element (vs. a custom component)- Whether
handleButtonClickis actually being triggered twice in practice- Whether any guards against double-activation already exist in the implementation
The review's recommendations remain sound: either rely on native
<button>behavior alone for keyboard activation, or implement explicit guards (viapreventDefault()on keyup for Space, or tracking state to prevent double-run). Manual verification in a browser with devtools is recommended as requested in the original comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/ProgramActions.tsx (1)
121-122: Optional: Consider addingaria-controlsto link trigger and menu.While not critical, you could add an
aria-controlsattribute to the trigger button to explicitly associate it with the menu for assistive technologies:ref={triggerRef} aria-haspopup="menu" aria-expanded={dropdownOpen} + aria-controls="program-actions-menu"And add the corresponding
idto the menu:<div + id="program-actions-menu" role="menu"This is a minor enhancement mentioned in the learnings but not essential—the current implementation is already fully functional and accessible.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ProgramActions.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/ProgramActions.tsx
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/ProgramActions.tsx
[warning] 135-139: Elements with the 'menu' interactive role must be focusable.
🔇 Additional comments (3)
frontend/src/components/ProgramActions.tsx (3)
21-21: Previous issues resolved: triggerRef properly assigned.The critical issue flagged in the previous review has been addressed. The
triggerRefis now correctly assigned to the trigger button (line 120), enabling the focus-return behavior on Escape (line 107).Also applies to: 120-120
68-74: Excellent keyboard navigation implementation following ARIA best practices.This implementation correctly follows the W3C ARIA Authoring Practices Guide for menu patterns:
- Auto-focus management (lines 68-74): First menu item receives focus when the menu opens, using
requestAnimationFrameto ensure the DOM is ready.- Roving tabindex pattern (lines 76-113): Arrow keys, Home, and End navigate between menu items with proper focus management.
- Focus return (line 107): Escape closes the menu and returns focus to the trigger.
- ARIA attributes (lines 121-122):
aria-haspopup="menu"andaria-expandedproperly communicate the menu state.- Keyboard activation (lines 124-129): Enter and Space toggle the dropdown on the trigger.
- Visual feedback (lines 130, 147):
focus-visibleprovides clear keyboard focus indicators.- Menu items (line 145):
tabIndex={-1}correctly removes items from the tab sequence while keeping them programmatically focusable.As per learnings, all essential accessibility features are present: ARIA attributes, keyboard support for Enter/Space/Escape, and click-outside handling.
Also applies to: 76-113, 120-130, 135-147
135-139: SonarCloud warning is a false positive—menu container should NOT be focusable.The static analysis warning about the menu container needing to be focusable is incorrect. According to the W3C ARIA Authoring Practices Guide, menu containers with
role="menu"should NOT be in the tab sequence or directly focusable. Focus management occurs programmatically on the individual menu items (which correctly usetabIndex={-1}for roving tabindex pattern).The previous review comment specifically requested removal of
tabIndex={0}from this container, which you've correctly implemented. The current implementation is compliant with ARIA standards.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/components/Release.tsx (1)
76-95: Simplify keyboard handling on native button to avoid duplicated behaviorAdding
type="button"and afocus-visiblering is great for accessibility, but for a semantic<button>you already get Enter/Space activation wired toonClickby the browser. The customonKeyDownhandler duplicates the navigation logic and reimplements the native activation behavior, which is unnecessary for WCAG compliance and slightly increases maintenance cost.I’d recommend relying solely on
onClickhere and removingonKeyDown(or extracting a sharedhandleNavigatefunction if you explicitly want low-level key handling elsewhere). Based on learnings, explicit key handlers are usually reserved for non-semantic clickable elements (e.g.,<div role="button">).frontend/src/components/ModuleList.tsx (1)
23-32: Align focus styles between module pills and “Show more” ButtonThe
focus-visible:ring-1on the module buttons is a good a11y improvement. For consistency with that behavior and the PR’s general “keyboard focus” theme, consider switching the show more/lessButtonfromfocus:ring-1tofocus-visible:ring-1as well (unless you intentionally want focus styling for mouse focus there).This keeps the visual focus indicator specifically tied to keyboard navigation across both controls.
Also applies to: 35-41
frontend/src/app/dev/page.tsx (1)
16-20: Reduce reliance onas anyfor undefined/null ModuleList scenariosThe dev page is very helpful for visually exercising ModuleList, but the
undefinedModules/nullModulesdefined as... as anybreak themodules: string[]type contract.If you plan to keep these scenarios long term, consider either:
- Widening the prop to something like
modules?: string[](and keeping the existing runtime guard), or- Restricting this dev page to valid
string[]inputs and covering undefined/null purely in unit tests.That avoids
anywhile still validating the edge cases you care about.Also applies to: 59-73
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/src/app/dev/page.tsx(1 hunks)frontend/src/components/ActionButton.tsx(2 hunks)frontend/src/components/Card.tsx(1 hunks)frontend/src/components/ModuleList.tsx(2 hunks)frontend/src/components/Release.tsx(2 hunks)frontend/src/components/Search.tsx(2 hunks)frontend/src/components/SortBy.tsx(3 hunks)frontend/src/components/ToggleableList.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/components/SortBy.tsx
- frontend/src/components/Card.tsx
- frontend/src/components/ActionButton.tsx
- frontend/src/components/ToggleableList.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/src/app/dev/page.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/app/dev/page.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/app/dev/page.tsx
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/Release.tsx
🪛 Biome (2.1.2)
frontend/src/components/Search.tsx
[error] 92-92: This JSX property is assigned multiple times.
This attribute is assigned again here.
(lint/suspicious/noDuplicateJsxProps)
🔇 Additional comments (1)
frontend/src/components/Search.tsx (1)
79-83: Nice accessible labelling for the search inputAssociating the visually-hidden
<label>with the input viahtmlFor="search-input"/id="search-input"is correct and improves screen reader support without affecting layout. This aligns well with the PR’s a11y goals.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Many of these are just partially covered for required criteria.
Here's an overview:
| Component | Keyboard Interaction | onKeyDown Handler | Focus Indicators | Tab Order | Status |
|---|---|---|---|---|---|
| ModuleList.tsx | ❌ Missing | ✅ Yes | ❌ NOT MET | ||
| ToggleableList.tsx | ✅ Yes | ✅ Yes | ✅ Yes | ✅ MET | |
| Search.tsx | ❌ Missing | ❌ Missing | ✅ Yes | ❌ NOT MET | |
| SortBy.tsx | ✅ Yes | ✅ Yes | ✅ Yes | ✅ MET | |
| EntityActions.tsx | ❌ Missing | ❌ Missing | ❌ Missing | ❌ NOT MET | |
| Release.tsx | ✅ Yes | ✅ Yes | ✅ Yes | ✅ MET | |
| Card.tsx | ✅ Yes | ✅ Yes | ✅ Yes | ✅ MET | |
| ShowMoreButton.tsx | ❌ Missing | ✅ Yes | ❌ NOT MET |
I also have some staying concerns that I left as requests ⬇️
There was a problem hiding this comment.
@sameersharmadev why was this added? Do we need this here? 🤔
There was a problem hiding this comment.
No no we don't. I forgot to check before I pushed the changes 😅
| label: 'font-medium text-sm text-gray-700 dark:text-gray-300 w-auto select-none pe-0', | ||
| trigger: | ||
| 'bg-transparent data-[hover=true]:bg-transparent focus:outline-none focus:ring-0 border-none shadow-none text-nowrap w-32 min-h-8 h-8 text-sm font-medium text-gray-800 dark:text-gray-200 hover:text-gray-900 dark:hover:text-gray-100 transition-all duration-0', | ||
| 'bg-transparent data-[hover=true]:bg-transparent focus:outline-none focus-visible:ring-1 focus-visible:ring-1 focus-visible:ring-offset-1 border-none shadow-none text-nowrap w-32 min-h-8 h-8 text-sm font-medium text-gray-800 dark:text-gray-200 hover:text-gray-900 dark:hover:text-gray-100 transition-all duration-0', |
There was a problem hiding this comment.
I’m not sure this looks quite right.
Is there a way we could make it a bit more subtle?
| aria-expanded={isExpanded} | ||
| aria-label={isExpanded ? 'Show less items' : 'Show more items'} | ||
| className="flex items-center bg-transparent px-0 text-blue-400 focus:outline-none focus-visible:ring-1 focus-visible:ring-offset-1" |
There was a problem hiding this comment.
| aria-expanded={isExpanded} | |
| aria-label={isExpanded ? 'Show less items' : 'Show more items'} | |
| className="flex items-center bg-transparent px-0 text-blue-400 focus:outline-none focus-visible:ring-1 focus-visible:ring-offset-1" | |
| aria-expanded={isExpanded} | |
| className="flex items-center bg-transparent px-0 text-blue-400 focus:outline-none focus-visible:ring-1 focus-visible:ring-offset-1" |
arial-label is redundant here - the inner button text will be used by screen readers.
The |
|
Noted. Working on these changes! |
2 participants
Proposed change
Improves keyboard accessibility in accordance to WCAG 2.1 for interactive elements.
Resolves #2655
Added keyboard activation (onKeyDown handling for Enter / Space) where appropriate.
Added visible focus styles (focus-visible / focus:ring classes) for keyboard focus indicators.
Checklist
make check-testlocally; all checks and tests passed.