Fixed Sonar issue Use <output> instead of the "status" role and Use N… #3642
Conversation
…ew array instead of array
WalkthroughThis PR addresses a Sonar accessibility issue by replacing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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: 3
🤖 Fix all issues with AI agents
In `@frontend/src/components/skeletons/Card.tsx`:
- Around line 19-21: In the Card skeleton component, the JSX currently uses an
<output> element to wrap block-level children (the element with className "flex
w-full justify-center" that contains the inner div with className "border-border
bg-card ..."); replace that outer <output> with a semantic <div> to ensure the
wrapper accepts block-level children and keeps the same className and behavior,
leaving the inner card div and its className unchanged.
- Around line 36-38: The render uses new
Array(numIcons).fill(...).map(...).map(...) which can throw RangeError for
negative or non-integer props; update the Card component to first sanitize
numIcons (e.g., coerce via Math.max(0, Math.floor(Number(numIcons) || 0)) or
default to 0) then emit the icons in a single pass using a safe array factory
(e.g., Array.from({ length: sanitizedNum }, (_, i) => <Skeleton
key={`icon-${i}`} .../>)); leave the local constant NUM_CONTRIBUTORS (always 8)
alone and apply the same single-pass pattern there if desired. Ensure keys use
the index-based `icon-${i}` and that Skeleton receives the same className.
In `@frontend/src/components/skeletons/UserCard.tsx`:
- Around line 11-13: The component is using an <output> element as the main
container (see UserCard component and the opening <output> at the top and its
closing tag near line 31); replace that outer <output> with a semantic block
container such as <div> to allow block-level children and valid HTML, and if you
still need an <output> for status/aria updates add a visually-hidden <output>
inside the container (or use an aria-live region) rather than using <output> as
the wrapper so markup and accessibility are correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/__tests__/unit/pages/Chapters.test.tsx (1)
106-118: Inconsistent test ID in the no-skeleton assertion.
queryByTestId('status')at Line 117 checks for a test ID that never existed—the original code usedrole="status", notdata-testid="status". This assertion passes trivially and doesn't verify skeletons are gone. Update it to match the pattern used inCommittees.test.tsx:Proposed fix
- expect(screen.queryByTestId('status')).not.toBeInTheDocument() + expect(screen.queryByTestId('card-skeleton')).not.toBeInTheDocument()frontend/__tests__/unit/pages/Users.test.tsx (1)
59-74: Inconsistent test ID in the no-skeleton assertion.Line 73 uses
queryByTestId('status')which never existed as a test ID. Update to verify user card skeletons are removed after data loads:Proposed fix
- expect(screen.queryByTestId('status')).not.toBeInTheDocument() + expect(screen.queryByTestId('user-card-skeleton')).not.toBeInTheDocument()frontend/__tests__/unit/pages/Projects.test.tsx (1)
72-84: Inconsistent test ID in the no-skeleton assertion.Line 83 uses
queryByTestId('status')which never existed as a test ID. Update to verify skeletons are removed:Proposed fix
- expect(screen.queryByTestId('status')).not.toBeInTheDocument() + expect(screen.queryByTestId('card-skeleton')).not.toBeInTheDocument()
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@asifrazadev hi! Left some comments. ⬇️
Please resolve conflicts and run make check-test . It is required as it says on the PR template.
| {showLevel && <Skeleton className="h-10 w-10 rounded-full" />} | ||
| <div className="flex flex-col gap-1"> | ||
| {showProjectName && <Skeleton className="h-8 w-[180px] sm:w-[250px]" />} | ||
| <div data-testid="card-skeleton" className="flex w-full justify-center"> |
There was a problem hiding this comment.
@asifrazadev do not add data-testid to actual code - this needs to be in the test.
| aria-live="polite" | ||
| aria-busy="true" | ||
| aria-label="Loading" | ||
| data-testid="user-card-skeleton" |
There was a problem hiding this comment.
@asifrazadev same for this, but also - I'm pretty sure the issue was about something else (switch from div to output) and that is not addressed here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/skeletons/Card.tsx">
<violation number="1" location="frontend/src/components/skeletons/Card.tsx:20">
P2: <output> elements can only contain phrasing content, but this output wraps block-level `<div>` content. This makes the HTML invalid and can confuse assistive tech. Use a `<div>` (or another flow container) for the status region instead of `<output>`.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/skeletons/UserCard.tsx">
<violation number="1" location="frontend/src/components/skeletons/UserCard.tsx:12">
P2: The <output> element only permits phrasing content, but it now wraps block-level <div> layout. This results in invalid HTML and can interfere with accessibility semantics for the live region. Consider moving the live-region <output> to a phrasing-only element (e.g., a visually hidden status text) and keep the layout container as a <div>.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@kasya i am sorry i am working on this but i dont know where i am making mistake i am stuck between 2 issue if
and and current sonar issue if nayone else wan you can re assign |
2 participants
Proposed change
Resolves #3557
Summary
Fixed multiple SonarQube code quality issues to improve accessibility, code maintainability, and testing practices.
Changes Made
Accessibility improvement: Replaced role="status" with role="output" to ensure better accessibility across all devices
Array initialization: Updated array creation to use the new Array() constructor instead of array literal syntax where applicable
React key prop: Removed usage of array index as React keys to prevent potential rendering issues
Test updates: Updated tests to reflect the above changes and ensure proper coverage
Checklist
make check-testlocally: all warnings addressed, tests passed