Add aria-labels to interactive elements for WCAG 2.1 compliance #2657
Conversation
Summary by CodeRabbit
WalkthroughAdded ARIA labels and a few interaction attributes to multiple frontend interactive elements; applied disabled to ToggleableList item buttons; updated unit and e2e tests to expect new accessible names and adjusted some wait/timeouts and matching strategies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
![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)
frontend/src/components/SortBy.tsx (1)
63-82: Invert the aria-label logic to describe the action, not the state.The
aria-labelcurrently describes the current state rather than the action the button will perform. WCAG guidelines require that button labels describe what happens when the button is activated.When
selectedOrder === 'asc', clicking the button will sort in descending order (Line 64), so the label should say"Sort in descending order", not"Sort in ascending order".Apply this diff to fix the logic:
aria-label={ - selectedOrder === 'asc' ? 'Sort in ascending order' : 'Sort in descending order' + selectedOrder === 'asc' ? 'Sort in descending order' : 'Sort in ascending order' }Note: The tooltip on Line 57 correctly describes the current state and should remain unchanged.
frontend/src/components/ProgramActions.tsx (1)
70-80: Keyboard navigation support missing – required for accessibility compliance.The component has appropriate ARIA attributes (
aria-label,aria-expanded,aria-haspopup,role="menuitem"), but lacks the keyboard interactions they imply. The button does not respond to Enter/Space keys, and the dropdown does not close on Escape. Additionally, focus management is not implemented—keyboard users cannot navigate menu items or have focus restored after the menu closes.Required fixes:
- Add
onKeyDownhandler to the button (lines 69-77) to toggle dropdown on Enter/Space- Add keyboard listener in
useEffect(lines 52-62) to close dropdown on Escape- Manage focus: move focus to first menu item when dropdown opens; restore focus to button when dropdown closes
🧹 Nitpick comments (1)
frontend/src/components/ToggleableList.tsx (1)
40-48: LGTM! Consider simplifying onClick handler.The
aria-labelanddisabledattributes improve accessibility. Thedisabledattribute correctly prevents interaction whenisDisabledis true.The onClick guard at Line 43 (
!isDisabled && handleButtonClick({ item })) is now redundant since thedisabledattribute prevents click events. Consider simplifying:- onClick={() => !isDisabled && handleButtonClick({ item })} + onClick={() => handleButtonClick({ item })}This is purely defensive code and not required for correctness.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/src/components/AnchorTitle.tsx(1 hunks)frontend/src/components/Card.tsx(1 hunks)frontend/src/components/ModuleList.tsx(1 hunks)frontend/src/components/Pagination.tsx(3 hunks)frontend/src/components/ProgramActions.tsx(1 hunks)frontend/src/components/Release.tsx(1 hunks)frontend/src/components/Search.tsx(1 hunks)frontend/src/components/SortBy.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.
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/src/components/AnchorTitle.tsxfrontend/src/components/ProgramActions.tsxfrontend/src/components/Card.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/ToggleableList.tsxfrontend/src/components/ProgramActions.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/ProgramActions.tsx
🔇 Additional comments (7)
frontend/src/components/Search.tsx (1)
88-94: LGTM!The
aria-label="Clear search"clearly describes the button's action for screen readers, improving accessibility for users relying on assistive technologies.frontend/src/components/Card.tsx (1)
114-127: LGTM!The
aria-labelwith a fallback (item.title || 'Social media link') ensures that each social media link is properly labeled for screen readers, even when the title is missing.frontend/src/components/AnchorTitle.tsx (1)
49-56: LGTM!The
aria-labelclearly describes the anchor link's purpose and destination, making the icon-only link accessible to screen readers.frontend/src/components/ModuleList.tsx (1)
23-32: LGTM! Note: pre-existing issue with non-interactive buttons.The
aria-labeladdition improves accessibility by providing context about each module.Note (pre-existing issue, not blocking): The module buttons (Lines 23-31) have no
onClickhandler, making them non-functional. If these are meant to be purely presentational, consider using<span>or<div>elements instead of<button>to avoid confusing keyboard and screen reader users. If they should be interactive, add appropriate click handlers.frontend/src/components/Pagination.tsx (3)
63-71: Excellent accessibility improvement for the Previous button!The descriptive aria-label provides clear context for screen reader users navigating the pagination controls.
83-96: Well-implemented dynamic aria-label for page buttons!The
aria-labeltransforms the minimal visible page number into a clear, descriptive action ("Go to page X") for assistive technologies, while the existingaria-currentproperly indicates the active page.
99-107: Excellent accessibility improvement for the Next button!The descriptive aria-label provides clear context for screen reader users, maintaining consistency with the Previous button implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/Release.tsx (1)
72-84: Consider addingtype="button"for clarity.While not required for this PR, explicitly setting
type="button"is a best practice that ensures the button won't accidentally submit a form if the component's context changes.Apply this diff:
<button + type="button" className="cursor-pointer overflow-hidden text-ellipsis whitespace-nowrap text-gray-600 hover:underline dark:text-gray-400" disabled={!release.organizationName || !release.repositoryName}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/Release.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
🔇 Additional comments (1)
frontend/src/components/Release.tsx (1)
81-81: LGTM! Descriptive aria-label improves accessibility.The aria-label clearly describes the button's action and includes a sensible fallback for missing repository names. This meets WCAG 2.1 requirements for accessible interactive elements.
|
@mirnumaan hi! Please run |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/Release.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
frontend/src/components/Release.tsx
Outdated
| if (!org || !repo) return | ||
| router.push(`/organizations/${org}/repositories/${repo}`) | ||
| }} | ||
| aria-label={`View repository ${release.repositoryName || 'Unknown'}`} |
There was a problem hiding this comment.
Make aria-label conditional on button state.
The aria-label correctly uses a fallback for undefined repositoryName, but there's a mismatch when the button is disabled. The button is disabled when either organizationName or repositoryName is missing (line 74), yet the aria-label always suggests an actionable "View repository" command. This can confuse screen reader users who encounter a disabled button with a label implying it can be activated.
Consider making the aria-label reflect the button's actual state:
- aria-label={`View repository ${release.repositoryName || 'Unknown'}`}
+ aria-label={
+ !release.organizationName || !release.repositoryName
+ ? 'Repository information unavailable'
+ : `View repository ${release.repositoryName}`
+ }This ensures screen reader users understand why the button is disabled when repository information is missing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| aria-label={`View repository ${release.repositoryName || 'Unknown'}`} | |
| aria-label={ | |
| !release.organizationName || !release.repositoryName | |
| ? 'Repository information unavailable' | |
| : `View repository ${release.repositoryName}` | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/Release.tsx around line 81, the aria-label always
reads "View repository ..." even when the button is disabled (disabled when
organizationName or repositoryName is missing); change the aria-label to reflect
the disabled state by conditionally rendering a non-actionable label when
disabled (for example "Repository information unavailable" or "View repository
[name]" only when organizationName and repositoryName are present), keep the
existing fallback for repositoryName when available, and ensure the same boolean
used to compute the disabled prop is reused for the aria-label check so screen
readers get an accurate status.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@mirnumaan thanks for working on this!
The changes look good, however you also need to update tests that we have - they are failing right now. Make sure to check locally with make check-test 👍🏼
* spelling fixes and tests * sonar and code rabbit suggestions implemented * json chunking and suggestions implemented * code rabbit and sonar qube suggestions * code rabbit suggestions * suggestions implemented * github advance security addressed * tests fixed * fixed tests * Clean up backend/test_commands.py --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]>
* Sync www-repopsitories (OWASP#2164) * spelling fixes and tests * sonar and code rabbit suggestions implemented * json chunking and suggestions implemented * code rabbit and sonar qube suggestions * code rabbit suggestions * suggestions implemented * github advance security addressed * tests fixed * fixed tests * Clean up backend/test_commands.py --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> * Consolidate code commits * Update cspell/custom-dict.txt * Update docker-compose/local.yaml * local yaml worder volume fix * instance check * poetry file updated --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]>
* fix slack and duplication errors * code rabbit suggestions * integrity error solved * using set * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]>
* improvements and refactoring * added prompt checks and tests for it
* question detection refining * sonar qube fixes * fix tests
* agentic rag * spelling fixes * code rabbit and sonar qube suggestions * code rabbit suggestions * refining * fix test * refining
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/About.test.tsx (1)
297-297: Use exact accessible name matcher instead of regex for consistency with e2e tests and stricter validation.The h2 heading's accessible name remains "Roadmap" after the aria-label addition to the anchor link (aria-labels on non-landmark elements don't affect parent heading names). The e2e test at
frontend/__tests__/e2e/pages/About.spec.ts:42expects exact match{ name: 'Roadmap' }, so the unit test should do the same. The regex/Roadmap/is unnecessarily permissive and could mask regressions if the heading's accessible name unexpectedly changes.Update line 297 to use the exact string match:
const roadmapSection = screen.getByRole('heading', { name: 'Roadmap' }).closest('div')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/components/ModuleList.test.tsx(4 hunks)frontend/__tests__/unit/components/Pagination.test.tsx(3 hunks)frontend/__tests__/unit/pages/About.test.tsx(1 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.
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
📚 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/__tests__/unit/components/Pagination.test.tsxfrontend/__tests__/unit/pages/About.test.tsx
🔇 Additional comments (8)
frontend/__tests__/unit/components/ModuleList.test.tsx (3)
6-42: LGTM! Mock implementations support aria-label correctly.The mock for
@heroui/buttonproperly passes through thearia-labelattribute (lines 22, 36), which is essential for the accessibility-focused test updates. Both mocks are well-structured for isolated unit testing.
182-228: LGTM! Truncation tests correctly updated for new aria-label pattern.The tests properly verify:
- Buttons use the full module name in
aria-labeleven when text is truncated (lines 190-193)- The
titleattribute is added for truncated modules (line 193, 216)- The
titleattribute is omitted for non-truncated modules (line 226)The use of a regex pattern at line 213-215 provides appropriate flexibility for testing truncation behavior.
230-269: LGTM! Button property tests consistently updated.The queries at lines 235 and 257 correctly use the new
Module: {moduleName}aria-label pattern. The tests continue to thoroughly verify button classes, attributes, and edge cases.frontend/__tests__/unit/components/Pagination.test.tsx (5)
44-45: LGTM: Prev/Next button ARIA labels are clear and consistent.The updated test selectors use descriptive ARIA labels ("Go to previous page" and "Go to next page") that clearly communicate the button actions to assistive technologies. The changes are applied consistently across all test cases covering enabled, disabled, and click behavior scenarios.
Also applies to: 58-58, 64-64, 71-71, 74-74, 116-117
52-52: LGTM: Page number button labels are specific and contextual.The test updates use the pattern "Go to page N" for all page number buttons, providing specific context about which page each button navigates to. The test on line 105 correctly verifies both the accessible name and the
aria-current="page"attribute on the active page button, which is excellent for accessibility testing.Also applies to: 77-77, 86-86, 95-95, 99-99, 105-105, 118-118, 126-126, 129-129
89-91: LGTM: Ellipses now have accessible labels.Adding the "More pages" label to ellipses (pagination truncation indicators) is a good accessibility improvement. Screen reader users will now understand that these elements represent additional pages that aren't displayed, rather than encountering unlabeled content.
135-135: LGTM: Regex pattern correctly matches page button labels.The regex pattern
/^Go to page (1|2)$/properly matches the new ARIA label format while using anchors to ensure exact matching. This is a clean approach for validating the presence of multiple similar elements.
1-137: All test expectations are met by the component implementation.Verification confirms that the Pagination component (frontend/src/components/Pagination.tsx) includes all ARIA labels and attributes that the tests expect: "Go to previous page" (line 68), "Go to next page" (line 104), "Go to page" pattern (line 86), "More pages" (line 78), and aria-current attributes (line 85). The test file is correctly aligned with the component implementation.
Summary for my edits :I found few more errors while editing the test file I have made changes, please let me know if you want to revert it or keep it , Thank you @kasya for assigning me the issue, looking forward to work on more issues . Pagination.test.tsxChanges:
Before:
After:
Reason :
About.test.tsxChanges:
Before:
After:
Reason:
ModuleList.test.tsxChanges :
Before
After
ProjectDetails.test.tsxChanges:-Added feature flag mock IS_PROJECT_HEALTH_ENABLED: true
Reason:
Home.test.tsxChanges:
Reason :
Users.spec.tsChanges:
Root Causes:
Reason :
###Kindly refer to screenshot for test results : 
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/__tests__/unit/pages/Home.test.tsx (1)
278-289: Good improvement: setTimeout replaced with waitFor.Replacing the blocking
setTimeoutwithwaitForis a testing best practice—it allows the test to pass as soon as the condition is met rather than waiting the full duration. The 3-second timeout appropriately buffers the 2-second animation.The
exact: falseapproach works for handling the separately rendered "+", though it could theoretically match values appearing elsewhere in the DOM.If you want to make the test more robust in the future, consider querying by test IDs or more specific selectors (e.g., within a stats container) rather than relying on partial text matches:
// Example alternative approach (for future consideration) const statsContainer = screen.getByTestId('stats-overview') // if such container exists for (const value of Object.values(stats)) { expect(within(statsContainer).getByText(millify(value), { exact: false })).toBeInTheDocument() }frontend/__tests__/unit/pages/ProjectDetails.test.tsx (1)
166-170: Consider more precise text matching or verify anchor links.The regex patterns accommodate AnchorTitle's anchor link additions but are quite loose (substring matching). This reduces test precision and could match unintended text.
Consider one of these approaches:
Option 1: Use more specific queries that verify both heading and anchor link
- expect(screen.getByText(/Issues Trend/)).toBeInTheDocument() + const issuesHeading = screen.getByRole('heading', { name: /Issues Trend/ }) + expect(issuesHeading).toBeInTheDocument() + expect(within(issuesHeading).getByRole('link', { name: /anchor/i })).toBeInTheDocument()Option 2: Use word boundaries in regex for stricter matching
- expect(screen.getByText(/Issues Trend/)).toBeInTheDocument() + expect(screen.getByText(/\bIssues Trend\b/)).toBeInTheDocument()Both approaches maintain test resilience while improving precision.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/e2e/pages/About.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Users.spec.ts(1 hunks)frontend/__tests__/unit/pages/Home.test.tsx(1 hunks)frontend/__tests__/unit/pages/ProjectDetails.test.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-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/__tests__/e2e/pages/About.spec.ts
🧬 Code graph analysis (1)
frontend/__tests__/e2e/pages/About.spec.ts (1)
frontend/__tests__/unit/data/mockAboutData.ts (1)
mockAboutData(1-50)
🔇 Additional comments (4)
frontend/__tests__/unit/pages/ProjectDetails.test.tsx (1)
40-42: Mock structure is correct and follows Jest best practices.The mock properly exports
IS_PROJECT_HEALTH_ENABLEDas a named export, matching the actual module atfrontend/src/utils/env.client.ts. Hardcoding the value totrueappropriately isolates the test from environment configuration while enabling the health metrics section for testing.frontend/__tests__/e2e/pages/Users.spec.ts (1)
44-44: LGTM! Test selector correctly updated to match new pagination aria-label.The selector change from a numeric button name to the descriptive "Go to page 2" aria-label aligns with the accessibility improvements in the Pagination component and maintains the same test behavior.
frontend/__tests__/e2e/pages/About.spec.ts (2)
39-42: Heading matchers correctly updated for AnchorTitle accessibility changes.The shift from exact string matching to regex patterns accommodates the anchor link content that AnchorTitle adds to headings' accessible names. Note that these unanchored regex patterns (e.g.,
/Roadmap/) will match any text containing the substring, which slightly reduces test precision but is acceptable given the accessibility improvements.Also applies to: 73-73
81-88: Counter assertions correctly match component structure; verify stat ordering aligns with test expectations.The split counter assertions match the actual component rendering in
frontend/src/app/about/page.tsx(lines 294–305), where each statistic is rendered with a label and formatted numeric value as separate elements. Usingexact: falsefor numeric values is appropriate for formatted counters (e.g., '1.2K+', '40+').Confirm the stat order in the test matches the component's grid rendering: Forks/Stars/Contributors/Open Issues.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@mirnumaan there are still some tests failing. Could you fix those? I also left some questions.
| await expect(page.getByRole('heading', { name: /Project Timeline/ })).toBeVisible() | ||
| await expect(page.getByRole('heading', { name: /Our Story/ })).toBeVisible() | ||
| await expect(page.getByRole('heading', { name: 'Leaders' })).toBeVisible() | ||
| await expect(page.getByRole('heading', { name: 'Roadmap' })).toBeVisible() | ||
| await expect(page.getByRole('heading', { name: /Roadmap/ })).toBeVisible() |
There was a problem hiding this comment.
Why did you change these to be a regex? 🤔
There was a problem hiding this comment.
I apologize for the confusion, I made an incorrect assumption about how the AnchorTitle component affects the accessible name of headings.
What I assumed was that when the AnchorTitle component adds an anchor link with aria-label="Link to {title} section" inside or near a heading, I thought the heading's accessible name would be affected and become something like:
"Project Timeline Link to Project Timeline section"
So I thought Playwright's getByRole('heading', { name: 'Project Timeline' }) would fail because it was looking for an exact match.
The Reason my approach/assumption was wrong because the aria-label on the anchor link only defines the accessible name for that specific link element, not for the parent heading.
| await expect(page.getByText('1.2K+', { exact: false })).toBeVisible() | ||
| await expect(page.getByText('Contributors')).toBeVisible() |
There was a problem hiding this comment.
Also why did you split these into 2 separate checks? 🤔
There was a problem hiding this comment.
I saw that the number+plus sign (1.2K+) and the label (Contributors) were rendered in two separate
await expect(page.getByText('1.2K+Contributors')).toBeVisible()
I apologize for the confusion, I was being overly cautious about the DOM structure when the original approach was correct.
I am trying to fix the test but every time it is giving me different results. |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/__tests__/e2e/pages/Chapters.spec.ts (1)
28-28: Inconsistent timeout strategy within the same test.Only the first assertion adds an explicit timeout, while subsequent assertions on lines 29-31 don't. If the page is genuinely slow to render, consider:
- Using
page.waitForLoadState('networkidle')orpage.waitForLoadState('domcontentloaded')in thebeforeEachhook instead of adding timeouts to individual assertions.- Waiting for a key element to be visible before proceeding with assertions, making the first wait act as a page-ready signal.
This approach is more maintainable than scattering timeouts across individual assertions.
frontend/__tests__/e2e/pages/ProjectsHealthDashboardMetrics.spec.ts (1)
24-24: Consider consolidating the wait strategy.Only the first data assertion has an explicit timeout. If health metrics data loads slowly, add a proper wait condition in the test setup rather than relying on the first assertion's timeout as a side-effect page-ready signal.
For example:
test('renders health metrics data', async ({ page }) => { await mockDashboardCookies(page, mockHealthMetricsData, true) await page.goto('/projects/dashboard/metrics') + await page.waitForLoadState('networkidle') const firstMetric = mockHealthMetricsData.projectHealthMetrics[0] - await expect(page.getByText(firstMetric.projectName)).toBeVisible({ timeout: 10000 }) + await expect(page.getByText(firstMetric.projectName)).toBeVisible()frontend/__tests__/e2e/pages/Contribute.spec.ts (1)
28-28: Same timeout pattern as other test files.This follows the same pattern: only the first assertion gets a timeout. See comments on other test files about consolidating wait strategies at the test setup level.
frontend/__tests__/e2e/pages/OrganizationDetails.spec.ts (1)
48-50: Consolidate wait strategy for milestones section.Apply the same consolidation recommendation as other test files. If milestones load slowly, consider waiting for the section to be ready before making assertions.
frontend/__tests__/e2e/pages/Home.spec.ts (1)
98-100: Duplicate timeout pattern.This follows the same first-assertion timeout pattern seen across multiple test files. Consider implementing a systematic wait strategy as suggested in other files rather than adding timeouts to individual assertions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/__tests__/e2e/pages/Chapters.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Contribute.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Home.spec.ts(2 hunks)frontend/__tests__/e2e/pages/OrganizationDetails.spec.ts(1 hunks)frontend/__tests__/e2e/pages/ProjectsHealthDashboardMetrics.spec.ts(1 hunks)frontend/playwright.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🔇 Additional comments (2)
frontend/playwright.config.ts (1)
18-20: The review comment's premise is incomplete—explicit timeouts are necessary for the chromium project, not redundant.The playwright.config.ts shows that
expect.timeout: 10000is configured at the project level only for Mobile Safari, not globally or for chromium. The 7 explicit{ timeout: 10000 }calls in test files are required for the chromium project to work correctly, since chromium has no timeout configured. They are only redundant for Mobile Safari tests.The valid concern is architectural: the current mixed approach (both project-level and explicit timeouts) creates ambiguity about intent. The design should clarify whether longer timeouts are needed for all projects (move to root config) or only Mobile Safari (remove explicit assertions and document why). The current setup contradicts itself rather than being purely redundant.
frontend/__tests__/e2e/pages/Home.spec.ts (1)
51-56: <!-- [scratchpad_start]
[task investigation_complete]
[observations]
- Home page (page.tsx) uses GraphQL query + Algolia fetch for geoLocData
- Page loading condition:
if (isLoading || !graphQLData || !geoLocData) return <LoadingSpinner />- Test mocks GraphQL endpoint BUT NOT Algolia endpoint
- Algolia fetch in useEffect is fire-and-forget (async without proper test integration)
- AnchorTitle contains aria-label only on anchor link, not heading (accessibility feature)
- Modified components have only scroll/focus/debounce useEffects, NO async data operations
- Test timeout only on heading, not on subsequent assertions (author, date)
- Other similar tests (chapters, projects) have NO timeout on their headings
[analysis]- Root cause: Test setup mocks GraphQL but not Algolia - geoLocData fetch is uncontrolled
- Page blocks rendering until BOTH graphQLData AND geoLocData load
- Algolia response timing is unpredictable in test environment, causing intermittent delays
- aria-label changes to AnchorTitle are accessibility improvements, not performance-related
- Modified components' useEffects are not causing the timing issue
- Timeout added to heading suggests developer worked around issue instead of fixing root cause
- Date assertion works eventually because all data has loaded by then
- Review comment's assessment is CORRECT: this is trial-and-error debugging
[pending]- None - verification complete
[done]
Root cause identified: Algolia fetch timing in test, not aria-label changes or modified component logic
</scratchpad_end] -->The timing issue stems from the uncontrolled Algolia data fetch, not the accessibility changes.
The test mocks the GraphQL endpoint but not the Algolia fetch that loads chapter data (
geoLocData). The page's render is blocked by the conditionif (isLoading || !graphQLData || !geoLocData), meaning it waits for both the mocked GraphQL data and the uncontrolled Algolia response. Once Algolia eventually responds, the page renders and all elements become visible—which is why the timeout on the heading works but isn't needed for subsequent assertions.The aria-label addition to the anchor in AnchorTitle is an accessibility improvement that doesn't impact rendering performance. The modified components (Search.tsx, AnchorTitle.tsx, SortBy.tsx, Pagination.tsx) contain only scroll navigation, focus management, and debounce cleanup—no async operations that would cause delays.
Recommended fixes:
- Mock the Algolia endpoint in the test setup, or adjust the loading condition to not block on
geoLocDataif it's not required for the "News & Opinions" section.- Remove the inconsistent timeout and ensure data loading is predictable in tests rather than working around timing issues.
|
Fixes that helped me with the test includes. One,I Increased default timeout from 5s to 10s for all Mobile Safari assertions. Second, increased some of the Individual test timeouts by adding explicit 10s timeouts in 6 problematic test files as extra insurance. 
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@mirnumaan thanks for working on this 👍🏼
Thank you so much for assigning 😊. It was so much fun to fix the errors and play with it |
I want to work on another issue, please assign to me if you have any. Since everything in the issues tab have been assigned. Kindly let me know . |
4 participants
Proposed change
Resolves #2653
Summary
Add descriptive
aria-labelattributes to interactive elements without visible text to improve accessibility and achieve WCAG 2.1 compliance.Fixed accessibility issues in 9 components by adding clear, descriptive
aria-labelattributes:aria-labelto section anchor link iconaria-label="Clear search"to clear buttonaria-labelto sort order toggle buttonaria-label,aria-expanded, andaria-haspopupto menu buttonaria-labelto social media icon linksaria-labeldescribing each module to module badge buttonsaria-labelanddisabledattribute to tag buttonsaria-labelto repository link buttonaria-labelto previous, next, and page number buttonsChecklist
make check-testlocally; all checks and tests passed.