Task/badge implementation in frontend #2273
Conversation
- Introduced BadgeNode for GraphQL representation of badges. - Enhanced UserNode with badges and badge count fields. - Updated UserCard to display badge count and integrated Badges component in UserDetailsPage. - Added tests for badge display and functionality across components. - Updated GraphQL queries to include badge data and counts.
- Improved formatting and readability in user_test.py by adjusting line breaks. - Updated mockBadgeData.ts to enhance consistency in object formatting. - Simplified Tooltip mock in Badges.test.tsx for better clarity. - Minor adjustments in Badges.tsx for consistent className formatting.
Summary by CodeRabbit
WalkthroughAdds backend badge_count resolver and adjusts badges resolver ordering/return items; expands GraphQL user selections to include badges and badgeCount; introduces frontend Badge component, types, UserCard badgeCount display, member page badge rendering, tests, fixtures, and cspell additions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
🧹 Nitpick comments (15)
cspell/custom-dict.txt (1)
59-59: Consider adding other common FA tokens to avoid future cspell noise.
Add: far, fab, fa-solid, fa-regular, fa-brands.fas +far +fab +fa-solid +fa-regular +fa-brandsfrontend/src/types/badge.ts (1)
1-7: Make Badge props readonly to prevent accidental mutation across components.-export type Badge = { +export type Badge = { - id: string - name: string - cssClass?: string - description?: string - weight: number + readonly id: string + readonly name: string + readonly cssClass?: string + readonly description?: string + readonly weight: number }frontend/src/app/members/[memberKey]/page.tsx (1)
212-226: Ensure icon fallback includes a style prefix and apply deterministic client-side sort as a safety net.
If backend sorting changes or a badge lacks cssClass, this keeps order and rendering stable.- {user?.badges && user.badges.length > 0 && ( + {user?.badges && user.badges.length > 0 && ( <div className="mt-2 flex flex-wrap gap-2"> <span className="text-sm text-gray-500 dark:text-gray-400">Badges:</span> - {user.badges.map((badge) => ( + {user.badges + .slice() + .sort((a, b) => (b.weight - a.weight) || a.name.localeCompare(b.name)) + .map((badge) => ( <Badges key={badge.id} name={badge.name} - cssClass={badge.cssClass || 'fa-medal'} + cssClass={badge.cssClass || 'fa-solid fa-medal'} description={badge.description || ''} weight={badge.weight} showTooltip={true} /> ))} </div> )}frontend/src/app/members/page.tsx (1)
44-45: Tiny stylistic nit.
Use nullish coalescing for arrays as well.- badges={user.badges || []} + badges={user.badges ?? []}backend/tests/apps/github/api/internal/nodes/user_test.py (1)
18-44: Add a schema-level test to assert camelCased GraphQL fields (badgeCount, cssClass).Backend types use snake_case (badge_count, css_class) while the frontend queries use camelCase; Strawberry will auto-convert snake_case to camelCase by default — add a small SDL/schema test that asserts the presence of "badgeCount" and "cssClass" in the generated schema. (github.com)
Relevant files to check: backend/settings/graphql.py, backend/apps/github/api/internal/nodes/user.py (badge_count), backend/apps/nest/api/internal/nodes/badge.py (css_class), frontend/src/server/queries/userQueries.ts.
frontend/src/components/UserCard.tsx (2)
72-77: Add accessible label and remove stray space.Provide an aria label for the medal icon and drop the trailing {' '} to avoid rendering a superfluous text node.
- {badgeCount > 0 && ( - <p className="mt-1 max-w-[250px] truncate text-sm text-gray-600 sm:text-base dark:text-gray-400"> - <FontAwesomeIcon icon={faMedal} className="mr-1 h-4 w-4" /> - {millify(badgeCount, { precision: 1 })}{' '} - </p> - )} + {badgeCount > 0 && ( + <p className="mt-1 max-w=[250px] truncate text-sm text-gray-600 sm:text-base dark:text-gray-400"> + <FontAwesomeIcon icon={faMedal} className="mr-1 h-4 w-4" aria-label="badges" /> + {millify(badgeCount, { precision: 1 })} + </p> + )}
36-36: Next/Image: use style for object-fit (Next 13+).
objectFitprop is deprecated; set via style to avoid console warnings.- <Image fill src={`${avatar}&s=160`} alt={name || 'user'} objectFit="cover" /> + <Image fill src={`${avatar}&s=160`} alt={name || 'user'} style={{ objectFit: 'cover' }} />backend/apps/github/api/internal/nodes/user.py (1)
41-45: Guard against duplicate rows in count.If the through table ever allows duplicates (migration or data drift),
.count()could overstate. Using.values("badge_id").distinct().count()is safer at minimal cost.- return self.badges.filter(is_active=True).count() + return ( + self.badges.filter(is_active=True) + .values("badge_id") + .distinct() + .count() + )frontend/__tests__/unit/components/UserCard.test.tsx (1)
113-142: Add assertion for medal icon in full-props case.Minor but helps ensure medal isn’t accidentally dropped when other stats render.
render(<UserCard {...fullProps} />) @@ expect(screen.getByText('25')).toBeInTheDocument() + expect(screen.getByTestId('icon-medal')).toBeInTheDocument()frontend/src/server/queries/userQueries.ts (1)
89-97: Consider over-fetching on some views.If certain pages only need
badgeCount, consider gating thebadges { ... }selection via separate queries or fragments to keep payloads small. Not blocking.frontend/src/components/Badges.tsx (2)
1-7: Bundle size caution: adding the entirefaspack is heavy.Registering all solid icons bloats the client bundle. If feasible, maintain a small allowlist (e.g., map backend
cssClassto a curated set) and add only those, or lazy-load uncommon icons.
16-21: Normalize multiple prefixes and odd inputs.Small hardening: strip repeated
fa-prefixes and convert underscores to dashes to match FA naming.- const safeCssClass = cssClass || 'fa-medal' - const iconName = String(safeCssClass).replace(/^fa-/, '') as IconName + const safeCssClass = (cssClass ?? 'fa-medal') as string + const iconName = String(safeCssClass).trim().replace(/^(?:fa-)+/, '').replace(/_/g, '-') as IconNamefrontend/__tests__/unit/pages/UserDetails.test.tsx (3)
18-29: Broaden FontAwesome mock to cover all IconProp shapes and avoid future brittlenessSupport string icons too and keep rest props separate from DOM-unsafe ones. This keeps the mock resilient to upstream changes.
-jest.mock('@fortawesome/react-fontawesome', () => ({ - FontAwesomeIcon: ({ - icon, - className, - ...props - }: { - icon: string[] | { iconName: string } - className?: string - [key: string]: unknown - }) => { - const iconName = Array.isArray(icon) ? icon[1] : icon.iconName - return <span data-testid={`icon-${iconName}`} className={className} {...props} /> - }, -})) +jest.mock('@fortawesome/react-fontawesome', () => ({ + FontAwesomeIcon: ({ + icon, + className, + ...rest + }: { + icon: string | [string, string] | { iconName: string } + className?: string + [key: string]: unknown + }) => { + const iconName = Array.isArray(icon) + ? icon[1] + : typeof icon === 'string' + ? icon + : icon.iconName + return <span data-testid={`icon-${iconName}`} className={className} {...rest} /> + }, +}))
32-56: Stabilize badge test-id slugging to reduce selector brittlenessCurrent slug only replaces whitespace; special chars remain (e.g., “badge-&-more!”). Consider mirroring production slugging (strip non-alphanumerics, collapse dashes) to avoid fragile selectors and potential collisions.
- const MockBadges = ({ + const MockBadges = ({ name, cssClass, showTooltip, }: { name: string cssClass: string showTooltip?: boolean }) => ( - <div - data-testid={`badge-${name.toLowerCase().replace(/\s+/g, '-')}`} + <div + data-testid={`badge-${name + .toLowerCase() + .replace(/[^a-z0-9]+/g, '-') + .replace(/^-+|-+$/g, '')}`} data-css-class={cssClass} data-show-tooltip={showTooltip} > <span data-testid={`icon-${cssClass.replace('fa-', '')}`} /> </div> )If you adopt this, please update the two tests that assert special-char IDs accordingly.
676-703: Optional: prefer a safer test-id for special charactersUsing raw special characters in data-testid can be error-prone in selectors and tooling. If you keep them, consider also asserting by visible name or role to reduce coupling to test-id formatting.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
backend/apps/github/api/internal/nodes/user.py(2 hunks)backend/apps/nest/api/internal/nodes/badge.py(1 hunks)backend/tests/apps/github/api/internal/nodes/user_test.py(3 hunks)cspell/custom-dict.txt(1 hunks)frontend/__tests__/unit/components/Badges.test.tsx(1 hunks)frontend/__tests__/unit/components/UserCard.test.tsx(3 hunks)frontend/__tests__/unit/data/mockBadgeData.ts(1 hunks)frontend/__tests__/unit/data/mockUserDetails.ts(1 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(2 hunks)frontend/src/app/members/[memberKey]/page.tsx(2 hunks)frontend/src/app/members/page.tsx(1 hunks)frontend/src/components/Badges.tsx(1 hunks)frontend/src/components/UserCard.tsx(3 hunks)frontend/src/server/queries/userQueries.ts(3 hunks)frontend/src/types/badge.ts(1 hunks)frontend/src/types/card.ts(2 hunks)frontend/src/types/user.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.
Applied to files:
backend/apps/nest/api/internal/nodes/badge.py
🧬 Code graph analysis (7)
frontend/src/types/card.ts (1)
frontend/src/types/badge.ts (1)
Badge(1-7)
frontend/src/types/user.ts (1)
frontend/src/types/badge.ts (1)
Badge(1-7)
backend/apps/github/api/internal/nodes/user.py (1)
backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(18-19)
backend/apps/nest/api/internal/nodes/badge.py (1)
frontend/src/types/badge.ts (1)
Badge(1-7)
backend/tests/apps/github/api/internal/nodes/user_test.py (2)
backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(18-19)backend/apps/github/api/internal/nodes/user.py (3)
badges(32-39)UserNode(28-69)badge_count(42-44)
frontend/__tests__/unit/data/mockBadgeData.ts (1)
frontend/src/types/badge.ts (1)
Badge(1-7)
frontend/__tests__/unit/pages/UserDetails.test.tsx (2)
frontend/__tests__/unit/data/mockUserDetails.ts (1)
mockUserDetailsData(1-83)backend/apps/github/api/internal/nodes/user.py (1)
badges(32-39)
🔇 Additional comments (14)
cspell/custom-dict.txt (1)
59-59: LGTM — avoids false positives for Font Awesome style token.frontend/src/types/card.ts (1)
3-3: LGTM — props surface aligned with types and pages.Also applies to: 80-82
frontend/__tests__/unit/data/mockUserDetails.ts (1)
16-33: Confirm Font Awesome version and update icon class.
fa-shield-alt is FA5; in FA6 the icon is shield-halved — use "fa-solid fa-shield-halved" (or "fas fa-shield-halved"); update frontend/tests/unit/data/mockUserDetails.ts (lines 16–33) to replace 'fa-shield-alt' or enable FA6 legacy aliases.frontend/src/app/members/page.tsx (1)
37-46: Prefer backend-provided badgeCount with nullish coalescing; verify Algolia 'users' index has the field.Use backend badgeCount when present; fall back to badges.length.
File: frontend/src/app/members/page.tsx (lines ~37-46)
- const badgeCount = user.badges?.length || 0 + const badgeCount = user.badgeCount ?? user.badges?.length ?? 0Ensure the Algolia "users" index includes badgeCount (or keep the fallback).
frontend/src/types/user.ts (1)
1-1: Type additions look good — GraphQL camelCase is enabled by default.
Schema is instantiated in backend/settings/graphql.py (schema = strawberry.Schema(...)) with no explicit auto_camel_case override; Strawberry converts snake_case field names to camelCase by default, so frontend fields (badgeCount, badges, avatarUrl) will be exposed as expected. (github.com)frontend/src/components/UserCard.tsx (1)
17-27: LGTM: prop addition and wiring look correct.
badgeCountis optional, default-falsy behavior is fine and won’t render when absent/0. Tests cover negative/undefined scenarios.backend/apps/nest/api/internal/nodes/badge.py (1)
1-20: LGTM: schema surface matches frontend needs.Fields map cleanly to GraphQL (auto-camelCase →
cssClass), and read-only exposure is appropriate.frontend/__tests__/unit/components/UserCard.test.tsx (1)
385-420: LGTM: badge count test coverage is solid.Positive, zero, negative, and large-number formatting cases are covered alongside combined metrics.
frontend/src/server/queries/userQueries.ts (2)
10-18: LGTM: server–client field naming aligns.
badges { id name description cssClass weight }andbadgeCountmatch backend’s auto-camelCased fields.
111-111: LGTM: metadata query exposes badgeCount for lightweight needs.frontend/__tests__/unit/data/mockBadgeData.ts (1)
1-64: LGTM: concise fixtures match type surface.Covers both with/without badges and aligns with
Badgeshape.backend/apps/github/api/internal/nodes/user.py (1)
31-40: Confirm relation semantics for self.badges (through-model vs direct M2M)Sandbox verification failed; confirm whether self.badges is a through-model (e.g., UserBadge) with is_active and a ForeignKey named badge (so user_badge.badge is valid). If it's a direct ManyToMany to Badge, select_related("badge") and user_badge.badge are incorrect and the resolver should be adjusted accordingly.
File: backend/apps/github/api/internal/nodes/user.py (lines 31–40)
frontend/__tests__/unit/pages/UserDetails.test.tsx (2)
521-533: Good coverage: presence of badges section and basic renderingThis validates the high-level rendering path without overspecifying implementation details. Nicely scoped.
598-656: Fallback cssClass behavior is exercised wellBoth undefined and empty string cases are covered; assertions on the propagated prop keep the test focused on UI contract rather than implementation.
Got it , I'll be doing it in morning |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
frontend/src/components/Badges.tsx
Outdated
| // Fallback to a default icon if the specified icon doesn't exist | ||
| return ( | ||
| <div className="inline-flex items-center"> | ||
| {showTooltip ? ( |
There was a problem hiding this comment.
With this approach there's a code duplication for the same icon.
Tooltip component from heroui has isDisabled property that we can use instead and have just one line of code.
There was a problem hiding this comment.
@Piyushrathoree why was this resolved? 😮 I don't see changes I requested in this comment.
You don't need a showTooltip ternary logic here. You can use have Tooltip wrapper around a single FA icon element and pass in showTooltip to isDisabled property in the Tooltip
frontend/src/components/Badges.tsx
Outdated
|
|
||
| return ( | ||
| <div className="inline-flex items-center"> | ||
| {showTooltip ? ( |
There was a problem hiding this comment.
Same here for code duplication.
frontend/src/components/Badges.tsx
Outdated
| try { | ||
| findIconDefinition(lookup) | ||
| iconFound = true | ||
| } catch { | ||
| iconFound = false | ||
| } |
There was a problem hiding this comment.
You should be able to simplify this without having to set iconFound separately. If icon is found - it's true else I believe it returns null (you'll need to verify though!)
So you could just return result of findIconDefinition() here
There was a problem hiding this comment.
Same here - this should not be marked as resoled if it wasn't addressed.
Did you try this out? If it's not possible - you need to respond with why this has not been applied
| @{user?.login} | ||
| </Link> | ||
| <p className="text-gray-600 dark:text-gray-400">{formattedBio}</p> | ||
| {user?.badges && user.badges.length > 0 && ( |
There was a problem hiding this comment.
Can we use badgeCount here instead? 🤔
|
Also please run |
thanks for this kate , I'll be solving all the issues soon and make sure this for next issues |
it is looking like this , please check it again . I've fixed it
|
There was a problem hiding this comment.
@Piyushrathoree I unresolved all my comments that I left after last review - none of those were addressed but for some reason they were marked as resolved.
Please do not resolve the comments - those are for PR reviewers to resolve only after they were addressed.
Feel free to resolve any bot's comments (and this is actually required), but not reviewer's comments.
frontend/src/components/Badges.tsx
Outdated
| // Fallback to a default icon if the specified icon doesn't exist | ||
| return ( | ||
| <div className="inline-flex items-center"> | ||
| {showTooltip ? ( |
There was a problem hiding this comment.
@Piyushrathoree why was this resolved? 😮 I don't see changes I requested in this comment.
You don't need a showTooltip ternary logic here. You can use have Tooltip wrapper around a single FA icon element and pass in showTooltip to isDisabled property in the Tooltip
frontend/src/components/Badges.tsx
Outdated
| try { | ||
| findIconDefinition(lookup) | ||
| iconFound = true | ||
| } catch { | ||
| iconFound = false | ||
| } |
There was a problem hiding this comment.
Same here - this should not be marked as resoled if it wasn't addressed.
Did you try this out? If it's not possible - you need to respond with why this has not been applied
|
Also, these two blocks need to be aligned. This is how they look in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
backend/apps/github/api/internal/nodes/user.py (1)
41-44: Prefer Count(distinct=True) aggregation for clarity.Functionally equivalent to values().distinct().count(), but more idiomatic and expresses COUNT(DISTINCT) directly.
+from django.db.models import Count @@ def badge_count(self) -> int: """Resolve badge count.""" - return self.user_badges.filter(is_active=True).values("badge_id").distinct().count() + return ( + self.user_badges.filter(is_active=True) + .aggregate(n=Count("badge_id", distinct=True))["n"] + )backend/tests/apps/github/api/internal/nodes/user_test.py (2)
86-100: Tighten interaction assertions for badge_count.Add an assertion that count() was invoked once to fully validate the call chain.
result = UserNode.badge_count(mock_user) assert result == 3 mock_badges_queryset.filter.assert_called_once_with(is_active=True) mock_badges_queryset.filter.return_value.values.assert_called_once_with("badge_id") mock_badges_queryset.filter.return_value.values.return_value.distinct.assert_called_once() + mock_badges_queryset.filter.return_value.values.return_value.distinct.return_value.count.assert_called_once_with()
129-191: Optional: avoid coupling test doubles to BadgeNode spec.Using spec=BadgeNode while setting .weight/.name may become brittle if the GraphQL type changes. Plain Mock objects suffice since sorting is exercised via queryset ordering, not object attributes.
- mock_badge_high_weight = Mock(spec=BadgeNode) + mock_badge_high_weight = Mock() @@ - mock_badge_medium_weight_a = Mock(spec=BadgeNode) + mock_badge_medium_weight_a = Mock() @@ - mock_badge_medium_weight_b = Mock(spec=BadgeNode) + mock_badge_medium_weight_b = Mock() @@ - mock_badge_low_weight = Mock(spec=BadgeNode) + mock_badge_low_weight = Mock()frontend/__tests__/unit/pages/UserDetails.test.tsx (7)
18-29: Harden FontAwesomeIcon mock to handle all IconProp casesCovers string icons and avoids
undefinedtestids.- }: { - icon: string[] | { iconName: string } + }: { + icon: string | string[] | { iconName?: string } className?: string [key: string]: unknown }) => { - const iconName = Array.isArray(icon) ? icon[1] : icon.iconName - return <span data-testid={`icon-${iconName}`} className={className} {...props} /> + const iconName = + Array.isArray(icon) ? icon[1] : typeof icon === 'string' ? icon : icon?.iconName + return <span data-testid={`icon-${iconName ?? 'unknown'}`} className={className} {...props} /> },
32-56: Make MockBadges resilient (no crash on undefined cssClass) and use a safer slugPrevents
.replaceonundefinedand stabilizes testids when names include punctuation.- <div - data-testid={`badge-${name.toLowerCase().replace(/\s+/g, '-')}`} + <div + data-testid={`badge-${name + .toLowerCase() + .replace(/[^a-z0-9]+/g, '-') + .replace(/^-|-$/g, '')}`} data-css-class={cssClass} data-show-tooltip={showTooltip} > - <span data-testid={`icon-${cssClass.replace('fa-', '')}`} /> + <span data-testid={`icon-${(cssClass ?? '').replace(/^fa-/, '')}`} />
274-276: Inconsistent heatmap mock shape vs beforeEachBeforeEach returns
{ contributions: { years: [...] } }; this test returns{ years: [...] }. Align to one schema to avoid brittle tests.- ;(fetchHeatmapData as jest.Mock).mockResolvedValue({ - years: [{ year: '2023' }], // Provide years data to satisfy condition in component - }) + ;(fetchHeatmapData as jest.Mock).mockResolvedValue({ + contributions: { years: [{ year: '2023' }] }, // Align with beforeEach + })
554-571: Also assert icon rendering to fully verify props → icon mappingStrengthens the test without coupling to implementation.
const contributorBadge = screen.getByTestId('badge-contributor') expect(contributorBadge).toHaveAttribute('data-css-class', 'fa-medal') expect(contributorBadge).toHaveAttribute('data-show-tooltip', 'true') + expect(screen.getByTestId('icon-medal')).toBeInTheDocument() const securityBadge = screen.getByTestId('badge-security-expert') expect(securityBadge).toHaveAttribute('data-css-class', 'fa-shield-alt') expect(securityBadge).toHaveAttribute('data-show-tooltip', 'true') + expect(screen.getByTestId('icon-shield-alt')).toBeInTheDocument()
573-593: Prefer queryAllByTestId with length assertionsAvoids false positives and supports multiple matches cleanly.
- expect(screen.queryByTestId(/^badge-/)).not.toBeInTheDocument() + expect(screen.queryAllByTestId(/^badge-/)).toHaveLength(0)
595-615: Same here: use queryAllByTestIdKeeps the pattern consistent.
- expect(screen.queryByTestId(/^badge-/)).not.toBeInTheDocument() + expect(screen.queryAllByTestId(/^badge-/)).toHaveLength(0)
696-724: Stabilize testid for special charactersIf you adopt the safer slug in MockBadges, update this expectation accordingly.
- expect(screen.getByTestId('badge-badge-&-more!')).toBeInTheDocument() + expect(screen.getByTestId('badge-badge-more')).toBeInTheDocument()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/github/api/internal/nodes/user.py(1 hunks)backend/tests/apps/github/api/internal/nodes/user_test.py(3 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(3 hunks)frontend/src/app/members/[memberKey]/page.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/members/[memberKey]/page.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
backend/tests/apps/github/api/internal/nodes/user_test.py (2)
backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(18-19)backend/apps/github/api/internal/nodes/user.py (3)
UserNode(28-69)badge_count(42-44)badges(32-39)
frontend/__tests__/unit/pages/UserDetails.test.tsx (2)
frontend/__tests__/unit/data/mockUserDetails.ts (1)
mockUserDetailsData(1-83)backend/apps/github/api/internal/nodes/user.py (1)
badges(32-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (11)
backend/apps/github/api/internal/nodes/user.py (2)
41-44: Thanks for fixing the .value(...) bug flagged earlier.The switch to .values("badge_id").distinct().count() resolves the runtime error noted previously.
33-39: Badges resolver OK — uniqueness enforcedUserBadge defines unique_together = ("badge", "user") in backend/apps/nest/models/user_badge.py (and migration backend/apps/nest/migrations/0004_userbadge.py); no deduping or model change required.
backend/tests/apps/github/api/internal/nodes/user_test.py (4)
21-24: Schema field coverage updated correctly.Including badge_count in expected_field_names matches the new resolver.
101-112: Empty badges path covered.Good guard ensuring graceful degradation when no badges exist.
113-128: Single badge path covered.Mocks and expectations are clear.
193-206: Active-only filter validated.Good verification that is_active=True is applied.
frontend/__tests__/unit/pages/UserDetails.test.tsx (5)
539-552: Badges presence test LGTMValidates both badges render for users with badges.
617-645: Fallback cssClass test LGTMEnsures production fallback is applied when cssClass is missing.
Confirm the page passes the fallback, not the mock, to avoid masking regressions later.
647-675: Empty cssClass fallback test LGTMCovers the empty-string edge case.
677-694: Order assertion now matches backend sort (weight desc, then name asc)This resolves the earlier note about reversed order.
725-755: Long-name badge test LGTMGood coverage for rendering long labels.
| ...jest.requireActual('next/navigation'), | ||
| useRouter: jest.fn(() => mockRouter), | ||
| useParams: () => ({ userKey: 'test-user' }), | ||
| useParams: () => ({ memberKey: 'test-user' }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Route param change LGTM; verify no stale tests use userKey
Looks consistent with [memberKey] routes.
🏁 Script executed:
#!/bin/bash
# Find mocks/usages still referencing userKey
rg -nP -C2 '\buseParams\([^)]*\)\s*=>\s*\(\s*{[^}]*userKey' --type ts --type tsx
rg -nP '\[userKey\]|\buserKey\b' --type ts --type tsxLength of output: 55
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search repo for any references to `userKey` or legacy route segments and show context.
rg -n -S -C2 'userKey' || true
rg -n -S -C2 'useParams' || true
rg -n -S -C2 '\[userKey\]' || true
rg -n -S -C2 'memberKey' || trueLength of output: 36351
Route param change LGTM — tests still mock userKey; update to memberKey
Found tests mocking useParams with userKey; change those mocks to memberKey to match the route rename.
- frontend/tests/unit/pages/Users.test.tsx — useParams: () => ({ userKey: 'test-user' })
- frontend/tests/unit/pages/Projects.test.tsx — useParams: () => ({ userKey: 'test-user' })
🤖 Prompt for AI Agents
In frontend/__tests__/unit/pages/UserDetails.test.tsx around line 69 (and also
update frontend/__tests__/unit/pages/Users.test.tsx and
frontend/__tests__/unit/pages/Projects.test.tsx where useParams is mocked), the
tests still mock the route param as userKey; change those mocks to return
memberKey instead (e.g. useParams: () => ({ memberKey: 'test-user' })) so they
match the renamed route parameter.
| """List badges assigned to the user sorted by weight and name.""" | ||
| user_badges = ( | ||
| self.user_badges.select_related("badge") | ||
| .filter(is_active=True) | ||
| .order_by( | ||
| "badge__weight", | ||
| "badge__name", | ||
| ) | ||
| self.user_badges.filter(is_active=True) | ||
| .select_related("badge") | ||
| .order_by("-badge__weight", "badge__name") |
There was a problem hiding this comment.
Please revert if you don't introduce any changes here
There was a problem hiding this comment.
i have removed the is_active field and the -badge_weight is for sorting it from big to small
There was a problem hiding this comment.
got it , fixed them
| </Link> | ||
| <p className="text-gray-600 dark:text-gray-400">{formattedBio}</p> | ||
| {user?.badges && user.badges.length > 0 && ( | ||
| <div className="mt-2 flex flex-wrap gap-2"> |
There was a problem hiding this comment.
Why do you need to sort them here is they come sorted from the backend?
frontend/src/app/members/page.tsx
Outdated
| onclick: () => handleButtonClick(user), | ||
| } | ||
|
|
||
| const badgeCount = user.badges?.length || 0 |
There was a problem hiding this comment.
Let's use backend endpoint value instead.
There was a problem hiding this comment.
I don't see any need for such complex logic for badges. It looks to me like ai-generated code and tbh it's a bad one. I also recommend checking FontAwesomeIconWrapper we have.
There was a problem hiding this comment.
got it , and also handled it properly and also used fontAwesomeIconWrapper properly
| const UserCard = ({ | ||
| avatar, | ||
| button, | ||
| badgeCount, |
There was a problem hiding this comment.
because in the user cards I'm displaying a single badge with the count of how many badge a user have .
There was a problem hiding this comment.
@Piyushrathoree I'm pretty sure Ark meant "why bAdgeCount comes after bUtton" :)
Let's sort these alphabetically - this is something we try to keep in mind at all times.
…DetailsPage" This reverts commit 5c69863.
…b.com/Piyushrathoree/Nest into task/badge-implementation-in-frontend
- Updated UserNode to remove unnecessary filters for badge retrieval and count. - Modified BadgeNode to inherit from Node and adjusted fields accordingly. - Enhanced unit tests for UserNode to reflect changes in badge logic. - Simplified Badges component by removing FontAwesome dependencies and using a wrapper for icon rendering. - Improved UserDetailsPage to utilize badgeCount directly from user data. - Adjusted UsersPage to reference badgeCount instead of calculating length. - Updated GraphQL types to ensure BadgeNode extends Node for consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/src/components/Badges.tsx (2)
12-20: Add accessible label when tooltip is disabled.Screen readers get no name when isDisabled is true.
If you prefer minimal change without the refactor above, add aria-label to each icon:
- <FontAwesomeIconWrapper icon={'fa-medal'} className="h-4 w-4" /> + <FontAwesomeIconWrapper icon={'fa-medal'} className="h-4 w-4" aria-label={name} /> @@ - <FontAwesomeIconWrapper icon={'fa-question'} className="h-4 w-4" /> + <FontAwesomeIconWrapper icon={'fa-question'} className="h-4 w-4" aria-label={name} /> @@ - <FontAwesomeIconWrapper icon={cssClass} className="h-4 w-4" /> + <FontAwesomeIconWrapper icon={cssClass} className="h-4 w-4" aria-label={name} />Also applies to: 35-41
11-11: Component renders a single badge; consider renaming to 'Badge'.Improves clarity for future callers; export alias can remain for backwards compatibility.
backend/apps/github/api/internal/nodes/user.py (2)
33-33: Clarify sort directions in the docstring (nit).- """List badges assigned to the user sorted by weight and name.""" + """List badges assigned to the user sorted by weight (desc) then name (asc)."""
39-42: Optional: use aggregate Count(distinct=...) for clarity.Add import and switch to aggregate:
@@ -import strawberry +import strawberry +from django.db.models import Count @@ def badge_count(self) -> int: """Resolve badge count.""" - return self.user_badges.values("badge_id").distinct().count() + return self.user_badges.aggregate( + n=Count("badge_id", distinct=True) + )["n"]backend/tests/apps/github/api/internal/nodes/user_test.py (1)
131-191: Sorted ordering test mirrors the resolver contract well. Consider a lightweight helper to reduce repetition.Tests are good. If you want to DRY them, extract a small factory for mock_user_badge and a fixture for mock_badges_queryset.
Example helper (optional, in the test module):
def make_user_badge(badge): ub = Mock() ub.badge = badge return ub
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (7)
backend/apps/github/api/internal/nodes/user.py(1 hunks)backend/tests/apps/github/api/internal/nodes/user_test.py(3 hunks)frontend/__tests__/unit/components/Badges.test.tsx(1 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(3 hunks)frontend/src/app/members/[memberKey]/page.tsx(3 hunks)frontend/src/app/members/page.tsx(1 hunks)frontend/src/components/Badges.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/app/members/page.tsx
- frontend/tests/unit/pages/UserDetails.test.tsx
- frontend/src/app/members/[memberKey]/page.tsx
- frontend/tests/unit/components/Badges.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/apps/github/api/internal/nodes/user_test.py (3)
backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(19-20)frontend/src/types/__generated__/graphql.ts (2)
BadgeNode(40-47)UserNode(859-881)backend/apps/github/api/internal/nodes/user.py (3)
UserNode(28-67)badge_count(40-42)badges(32-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL
🔇 Additional comments (9)
frontend/src/components/Badges.tsx (2)
22-41: Fix FA style detection and guard lookup; skip FA check for custom icons.
- Hard‑coding 'fas' breaks 'far'/'fab' and tokens like 'fa-solid fa-crown'.
- findIconDefinition throws when missing; current code can crash the render.
- Custom/local (non‑FA) cssClass (OWASP‑specific) should render as‑is, not be forced to 'fa-question'.
Apply this diff to robustly parse style+icon, guard the lookup, and avoid false fallbacks for custom icons. Also dedup the JSX and add aria-label for a11y:
- const iconDef = findIconDefinition({ - prefix: 'fas', - iconName: cssClass.split(' ').pop().replace('fa-', '') as IconName, - }) - if (!iconDef) { - return ( - <div className="inline-flex items-center"> - <Tooltip content={name} isDisabled={!showTooltip}> - <FontAwesomeIconWrapper icon={'fa-question'} className="h-4 w-4" /> - </Tooltip> - </div> - ) - } - return ( - <div className="inline-flex items-center"> - <Tooltip content={name} isDisabled={!showTooltip}> - <FontAwesomeIconWrapper icon={cssClass} className="h-4 w-4" /> - </Tooltip> - </div> - ) + // Parse FA tokens; support 'fas/fa-solid', 'far/fa-regular', 'fab/fa-brands'. + const safeCss = String(cssClass).trim() + const tokens = safeCss.split(/\s+/) + const styleToken = tokens.find((t) => /^(?:fa-(solid|regular|brands)|fas|far|fab)$/i.test(t)) + const stylePrefix = + styleToken?.toLowerCase() === 'far' || styleToken?.toLowerCase() === 'fa-regular' + ? 'far' + : styleToken?.toLowerCase() === 'fab' || styleToken?.toLowerCase() === 'fa-brands' + ? 'fab' + : 'fas' + const faIconToken = tokens.find( + (t) => /^fa-[a-z0-9-]+$/i.test(t) && !/^fa-(solid|regular|brands)$/i.test(t), + ) + + // Default: trust custom/local classes; only FA‑lookup when we actually have an FA token. + let iconToRender = cssClass + if (faIconToken) { + const iconName = faIconToken.replace(/^fa-/, '') as IconName + try { + findIconDefinition({ prefix: stylePrefix, iconName }) + } catch { + iconToRender = 'fa-question' + } + } + + return ( + <div className="inline-flex items-center"> + <Tooltip content={name} isDisabled={!showTooltip}> + <FontAwesomeIconWrapper icon={iconToRender} className="h-4 w-4" aria-label={name} /> + </Tooltip> + </div> + )
1-3: Confirmed — FontAwesome icons are registered; no action required.frontend/src/utils/data.ts calls library.add(...) (line 36) and imports from @fortawesome/free-solid-svg-icons, @fortawesome/free-regular-svg-icons and @fortawesome/free-brands-svg-icons; unit tests also call library.add.
backend/apps/github/api/internal/nodes/user.py (2)
39-42: Fix confirmed: distinct count now uses .values(...).distinct().count().
33-37: Good resolver — uniqueness confirmedselect_related + ordering is correct. UserBadge.Meta declares unique_together = ("badge", "user") in backend/apps/nest/models/user_badge.py (also present in backend/apps/nest/migrations/0004_userbadge.py), so duplicates are prevented at the DB level.
backend/tests/apps/github/api/internal/nodes/user_test.py (5)
8-8: Importing BadgeNode for spec typing is appropriate.
21-24: Schema field list updated correctly to include badge_count.
86-98: badge_count test is precise and verifies the intended queryset chain.
99-112: Empty badges case covered; also asserts select_related/order_by usage.
113-130: Single-badge path exercised; spec usage keeps mocks tight.
|
hey @arkid15r , I've addressed the problems and also tested it , please review it and please share your feedback. |
| const UserCard = ({ | ||
| avatar, | ||
| button, | ||
| badgeCount, |
There was a problem hiding this comment.
@Piyushrathoree I'm pretty sure Ark meant "why bAdgeCount comes after bUtton" :)
Let's sort these alphabetically - this is something we try to keep in mind at all times.
| name | ||
| description | ||
| cssClass | ||
| weight |
There was a problem hiding this comment.
Where do we use badge weight on the frontend? 🤔
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/Badges.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
frontend/src/components/Badges.tsx
Outdated
| const iconDef = findIconDefinition({ | ||
| prefix: 'fas', | ||
| iconName: cssClass.split(' ').pop().replace('fa-', '') as IconName, | ||
| }) | ||
| return ( | ||
| <div className="inline-flex items-center"> | ||
| <Tooltip content={name} isDisabled={!showTooltip}> | ||
| <FontAwesomeIconWrapper icon={iconDef ? cssClass : 'fa-question'} className="h-4 w-4" /> |
There was a problem hiding this comment.
Respect badge style classes when resolving icons.
findIconDefinition is hard-coded to 'fas', so any fa-regular/fa-brands badge (e.g., fa-brands fa-github) fails the lookup and we always fall back to the question mark. That breaks the requirement to render the exact badge supplied by the backend.
Please derive the prefix from the class list (map fa-solid/fa-regular/fa-brands or their short aliases to 'fas' | 'far' | 'fab') and normalize the icon token instead of assuming the last segment. For example:
- const iconDef = findIconDefinition({
- prefix: 'fas',
- iconName: cssClass.split(' ').pop().replace('fa-', '') as IconName,
- })
+ const tokens = cssClass.trim().split(/\s+/).filter(Boolean)
+ let prefix: 'fas' | 'far' | 'fab' = 'fas'
+ for (const token of tokens) {
+ const normalized = token.toLowerCase()
+ if (normalized === 'fa-regular' || normalized === 'far') {
+ prefix = 'far'
+ } else if (normalized === 'fa-brands' || normalized === 'fab') {
+ prefix = 'fab'
+ } else if (normalized === 'fa-solid' || normalized === 'fas') {
+ prefix = 'fas'
+ }
+ }
+ const iconToken =
+ [...tokens]
+ .reverse()
+ .find((token) => /^fa-[a-z0-9-]+$/i.test(token) && !/^fa-(solid|regular|brands)$/i.test(token)) ?? 'fa-medal'
+ const iconName = iconToken.replace(/^fa-/, '').replace(/_/g, '-') as IconName
+
+ const iconDef = findIconDefinition({
+ prefix,
+ iconName,
+ })That ensures brand and regular badges render instead of degrading to the fallback.
📝 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.
| const iconDef = findIconDefinition({ | |
| prefix: 'fas', | |
| iconName: cssClass.split(' ').pop().replace('fa-', '') as IconName, | |
| }) | |
| return ( | |
| <div className="inline-flex items-center"> | |
| <Tooltip content={name} isDisabled={!showTooltip}> | |
| <FontAwesomeIconWrapper icon={iconDef ? cssClass : 'fa-question'} className="h-4 w-4" /> | |
| // Derive style prefix and icon name from the full cssClass tokens | |
| const tokens = cssClass.trim().split(/\s+/).filter(Boolean) | |
| let prefix: 'fas' | 'far' | 'fab' = 'fas' | |
| for (const token of tokens) { | |
| const normalized = token.toLowerCase() | |
| if (normalized === 'fa-regular' || normalized === 'far') { | |
| prefix = 'far' | |
| } else if (normalized === 'fa-brands' || normalized === 'fab') { | |
| prefix = 'fab' | |
| } else if (normalized === 'fa-solid' || normalized === 'fas') { | |
| prefix = 'fas' | |
| } | |
| } | |
| const iconToken = | |
| [...tokens] | |
| .reverse() | |
| .find((t) => /^fa-[a-z0-9-]+$/i.test(t) && !/^fa-(solid|regular|brands)$/i.test(t)) | |
| ?? 'fa-medal' | |
| const iconName = iconToken.replace(/^fa-/, '').replace(/_/g, '-') as IconName | |
| const iconDef = findIconDefinition({ | |
| prefix, | |
| iconName, | |
| }) | |
| return ( | |
| <div className="inline-flex items-center"> | |
| <Tooltip content={name} isDisabled={!showTooltip}> | |
| <FontAwesomeIconWrapper icon={iconDef ? cssClass : 'fa-question'} className="h-4 w-4" /> |
🤖 Prompt for AI Agents
In frontend/src/components/Badges.tsx around lines 22 to 29, the code always
uses prefix 'fas' when calling findIconDefinition which breaks
fa-regular/fa-brands badges; parse the cssClass string for the FontAwesome style
token (accept full names fa-solid/fa-regular/fa-brands and the short aliases
fa-solid->'fas', fa-regular->'far', fa-brands->'fab'), determine the correct
prefix to pass to findIconDefinition, and normalize the icon name by stripping
any leading 'fa-' and taking the token that represents the icon (not always
simply .split(' ').pop()). Keep the existing fallback to the question mark if
the resolved iconDefinition is missing.
Piyushrathoree
force-pushed
the
task/badge-implementation-in-frontend
branch
from
74f7704 to
ac27f26
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/src/types/__generated__/userQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (3)
frontend/src/components/Badges.tsx(1 hunks)frontend/src/components/UserCard.tsx(3 hunks)frontend/src/server/queries/userQueries.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/UserCard.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (6)
frontend/src/server/queries/userQueries.ts (5)
10-16: Confirm backend exposure ofbadgesselection.Pulling
badges { id name description cssClass }from theuserfield assumes every backendGetLeaderDataresolver now fetches and authorises this nested relation. Please double-check the matching GraphQL schema/resolvers before merging so we don’t ship a query that 500s in production.
16-16:badgeCountmust exist on the backend schema.Same concern here—if the backend has not yet deployed the
badgeCountfield onUser, this addition will break the query at runtime. Please verify the field is live (or keep the change behind a feature flag).
88-94: Ensurebadgesis included in User resolver.The broader
GET_USER_DATApayload now fetches badge metadata. Confirm the backend resolvers and permissions expose this for the authenticated viewer; otherwise we’ll regress this query.
94-94: VerifybadgeCountresolver availability.Same as above—the
badgeCountfield needs to be in the deployed schema; please validate before shipping.
109-109:badgeCountfield must exist in the metadata path.Fetching
badgeCounthere will fail if the metadata resolver path was not updated. Double-check backend parity.frontend/src/components/Badges.tsx (1)
22-25: Respect Font Awesome style prefixes.
findIconDefinitionis hard-coded to the solid prefix ('fas'). Brand and regular badges (e.g.,fa-brands fa-github,fa-regular fa-heart) will be treated as missing and always fall back to the question icon, breaking the badge display. ParsecssClassto derive the correct prefix ('fas' | 'far' | 'fab') before the lookup.- const iconDef = findIconDefinition({ - prefix: 'fas', - iconName: cssClass.split(' ').pop().replace('fa-', '') as IconName, - }) + const tokens = cssClass.trim().split(/\s+/).filter(Boolean) + let prefix: 'fas' | 'far' | 'fab' = 'fas' + for (const token of tokens) { + if (/^(fa-solid|fas)$/i.test(token)) prefix = 'fas' + else if (/^(fa-regular|far)$/i.test(token)) prefix = 'far' + else if (/^(fa-brands|fab)$/i.test(token)) prefix = 'fab' + } + const iconToken = + [...tokens] + .reverse() + .find((token) => /^fa-[a-z0-9-]+$/i.test(token) && !/^(fa-solid|fa-regular|fa-brands|fas|far|fab)$/i.test(token)) ?? 'fa-medal' + const iconName = iconToken.replace(/^fa-/, '') as IconName + const iconDef = findIconDefinition({ prefix, iconName })
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
frontend/src/components/Badges.tsx (1)
5-12: Align prop typing with runtime checks and backend data.
cssClassis required in the type but you handle it as optional/null at runtime. Make it optional to match upstream models and remove type noise at call sites.-type BadgeProps = { - name: string - cssClass: string - showTooltip?: boolean -} +type BadgeProps = { + name: string + cssClass?: string | null + showTooltip?: boolean +} -const Badges = ({ name, cssClass, showTooltip = true }: BadgeProps) => { +const Badges = ({ name, cssClass, showTooltip = true }: BadgeProps) => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/Badges.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
frontend/src/components/Badges.tsx
[error] 37-37: Expected an expression but instead found '<'.
Expected an expression here.
(parse)
🪛 GitHub Actions: Run CI/CD
frontend/src/components/Badges.tsx
[error] 37-37: Declaration or statement expected. (SyntaxError) in Badges.tsx while running 'prettier --log-level warn --write .'.
🔇 Additional comments (2)
frontend/src/components/Badges.tsx (2)
1-4: Register all FA packs used by badges so lookups don’t always fall back.Only solid ('fas') is implied; without registering 'far' and 'fab',
findIconDefinitionwill fail for regular/brands, causing false fallbacks.Apply:
-import { IconName, findIconDefinition } from '@fortawesome/fontawesome-svg-core' -import { Tooltip } from '@heroui/tooltip' -import FontAwesomeIconWrapper from 'wrappers/FontAwesomeIconWrapper' +import { IconName, IconLookup, findIconDefinition, library } from '@fortawesome/fontawesome-svg-core' +import { fas } from '@fortawesome/free-solid-svg-icons' +import { far } from '@fortawesome/free-regular-svg-icons' +import { fab } from '@fortawesome/free-brands-svg-icons' +import { Tooltip } from '@heroui/tooltip' +import FontAwesomeIconWrapper from 'wrappers/FontAwesomeIconWrapper' + +// Register packs so findIconDefinition works for all styles +library.add(fas, far, fab)If packs are already added globally, keep this out and confirm registry is in a shared module.
To verify global registration and avoid duplicate adds, run:
#!/bin/bash rg -nP --type=ts -C2 "library\.add\s*\(" rg -nP --type=ts "@fortawesome/free-(solid|regular|brands)-svg-icons"
22-25: Don’t hard-code 'fas' or rely on split().pop(); parse tokens robustly.Current logic breaks for 'fa-regular/fa-brands' or when the last token is a style token. The combined patch above (Lines 27–39) replaces this with style-aware parsing.
frontend/src/components/Badges.tsx
Outdated
| return ( | ||
| <div className="inline-flex items-center"> | ||
| <Tooltip content={name} isDisabled={!showTooltip}> | ||
| <FontAwesomeIconWrapper icon={'fa-medal'} className="h-4 w-4" /> |
There was a problem hiding this comment.
Ensure fallback includes a style prefix.
Use a full class with style to avoid ambiguous rendering.
- <FontAwesomeIconWrapper icon={'fa-medal'} className="h-4 w-4" />
+ <FontAwesomeIconWrapper icon={'fa-solid fa-medal'} className="h-4 w-4" />📝 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.
| <FontAwesomeIconWrapper icon={'fa-medal'} className="h-4 w-4" /> | |
| <FontAwesomeIconWrapper icon={'fa-solid fa-medal'} className="h-4 w-4" /> |
🤖 Prompt for AI Agents
In frontend/src/components/Badges.tsx around line 16, the fallback icon class
'fa-medal' is missing a FontAwesome style prefix which can cause ambiguous
rendering; update the icon prop to include the full style class (e.g., 'fas
fa-medal' or 'fa-solid fa-medal' to match the project's FA version) so the
fallback uses the correct style, and ensure any other fallback usages follow the
same prefixed pattern.
| return ( | ||
| <div className="inline-flex items-center"> | ||
| <Tooltip content={name} isDisabled={!showTooltip}> | ||
| <FontAwesomeIconWrapper | ||
| icon={iconDef ? `${prefix} fa-${iconName}` : 'fa-solid fa-question'} | ||
| className="h-4 w-4" | ||
| /> | ||
| </Tooltip> | ||
| </div> | ||
| ) | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Fix JSX syntax errors and undefined identifiers in render block.
There are stray closing tags (Lines 37–38) and usage of undefined variables prefix and iconName (Line 31), causing the build to fail. Replace the lookup/render logic to derive and use well-defined variables and close JSX properly.
Apply:
- const iconDef = findIconDefinition({
- prefix: 'fas',
- iconName: cssClass.split(' ').pop().replace('fa-', '') as IconName,
- })
-
- return (
- <div className="inline-flex items-center">
- <Tooltip content={name} isDisabled={!showTooltip}>
- <FontAwesomeIconWrapper
- icon={iconDef ? `${prefix} fa-${iconName}` : 'fa-solid fa-question'}
- className="h-4 w-4"
- />
- </Tooltip>
- </div>
- )
- </div>
- )
+ // Parse cssClass to derive FA style + icon
+ const safeCss = String(cssClass).trim()
+ const tokens = safeCss.split(/\s+/).filter(Boolean)
+
+ // Map style tokens to lookup prefix and CSS prefix
+ let faLookupPrefix: 'fas' | 'far' | 'fab' = 'fas'
+ let cssPrefix = 'fa-solid'
+ for (const t of tokens) {
+ const n = t.toLowerCase()
+ if (n === 'fa-regular' || n === 'far') {
+ faLookupPrefix = 'far'
+ cssPrefix = 'fa-regular'
+ } else if (n === 'fa-brands' || n === 'fab') {
+ faLookupPrefix = 'fab'
+ cssPrefix = 'fa-brands'
+ } else if (n === 'fa-solid' || n === 'fas') {
+ faLookupPrefix = 'fas'
+ cssPrefix = 'fa-solid'
+ }
+ }
+
+ // Pick an icon token (ignore style tokens), default to medal
+ const iconToken =
+ [...tokens].reverse().find((t) => /^fa-[a-z0-9-]+$/i.test(t) && !/^fa-(solid|regular|brands)$/i.test(t)) ??
+ 'fa-medal'
+ const iconName = iconToken.replace(/^fa-/, '').replace(/_/g, '-') as IconName
+ const renderClass = `${cssPrefix} fa-${iconName}`
+
+ // Will be null/undefined if not found; boolean check is enough
+ const iconDef = findIconDefinition({ prefix: faLookupPrefix as IconLookup['prefix'], iconName })
+
+ return (
+ <div className="inline-flex items-center">
+ <Tooltip content={name} isDisabled={!showTooltip}>
+ <FontAwesomeIconWrapper icon={iconDef ? renderClass : 'fa-solid fa-question'} className="h-4 w-4" />
+ </Tooltip>
+ </div>
+ )📝 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.
| return ( | |
| <div className="inline-flex items-center"> | |
| <Tooltip content={name} isDisabled={!showTooltip}> | |
| <FontAwesomeIconWrapper | |
| icon={iconDef ? `${prefix} fa-${iconName}` : 'fa-solid fa-question'} | |
| className="h-4 w-4" | |
| /> | |
| </Tooltip> | |
| </div> | |
| ) | |
| </div> | |
| ) | |
| } | |
| // Parse cssClass to derive FA style + icon | |
| const safeCss = String(cssClass).trim() | |
| const tokens = safeCss.split(/\s+/).filter(Boolean) | |
| // Map style tokens to lookup prefix and CSS prefix | |
| let faLookupPrefix: 'fas' | 'far' | 'fab' = 'fas' | |
| let cssPrefix = 'fa-solid' | |
| for (const t of tokens) { | |
| const n = t.toLowerCase() | |
| if (n === 'fa-regular' || n === 'far') { | |
| faLookupPrefix = 'far' | |
| cssPrefix = 'fa-regular' | |
| } else if (n === 'fa-brands' || n === 'fab') { | |
| faLookupPrefix = 'fab' | |
| cssPrefix = 'fa-brands' | |
| } else if (n === 'fa-solid' || n === 'fas') { | |
| faLookupPrefix = 'fas' | |
| cssPrefix = 'fa-solid' | |
| } | |
| } | |
| // Pick an icon token (ignore style tokens), default to medal | |
| const iconToken = | |
| [...tokens] | |
| .reverse() | |
| .find((t) => /^fa-[a-z0-9-]+$/i.test(t) && !/^fa-(solid|regular|brands)$/i.test(t)) ?? | |
| 'fa-medal' | |
| const iconName = iconToken.replace(/^fa-/, '').replace(/_/g, '-') as IconName | |
| const renderClass = `${cssPrefix} fa-${iconName}` | |
| // Will be null/undefined if not found; boolean check is enough | |
| const iconDef = findIconDefinition({ | |
| prefix: faLookupPrefix as IconLookup['prefix'], | |
| iconName, | |
| }) | |
| return ( | |
| <div className="inline-flex items-center"> | |
| <Tooltip content={name} isDisabled={!showTooltip}> | |
| <FontAwesomeIconWrapper | |
| icon={iconDef ? renderClass : 'fa-solid fa-question'} | |
| className="h-4 w-4" | |
| /> | |
| </Tooltip> | |
| </div> | |
| ) |
🧰 Tools
🪛 Biome (2.1.2)
[error] 37-37: Expected an expression but instead found '<'.
Expected an expression here.
(parse)
🪛 GitHub Actions: Run CI/CD
[error] 37-37: Declaration or statement expected. (SyntaxError) in Badges.tsx while running 'prettier --log-level warn --write .'.
🤖 Prompt for AI Agents
In frontend/src/components/Badges.tsx around lines 27 to 39, the render block
contains stray closing tags and references undefined identifiers `prefix` and
`iconName`; fix by deriving a single well-defined iconClass variable from the
incoming prop (e.g., if iconDef exists use `${iconDef.prefix}
fa-${iconDef.iconName}` else fallback to 'fa-solid fa-question'), remove the
extra two closing JSX tags so the component returns one properly closed root
element, and use iconClass in the FontAwesomeIconWrapper; ensure any dependent
props (showTooltip, name, iconDef) are referenced as they are declared in the
component signature.
…n (e.g., fa-crown for fa-solid). The frontend dynamically extracts the correct prefix for lookup and rendering. This is a critical design for backend compatibility,they are only fa-solid icons so badges.tsx only checks for fa-solid not anyother families. fix the coderabbit issue with syntax. also run make check-test
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/components/Badges.tsx (2)
1-1: Remove unused svg-core import.After dropping the lookup, this import is dead and should be deleted.
Apply this diff:
-import { IconName, findIconDefinition } from '@fortawesome/fontawesome-svg-core'
11-36: Optional: collapse to a single return with a resolved icon.Removes branching and avoids future duplication.
Apply this diff:
-const Badges = ({ name, cssClass, showTooltip = true }: BadgeProps) => { - if (!cssClass || typeof cssClass !== 'string') { - return ( - <div className="inline-flex items-center"> - <Tooltip content={name} isDisabled={!showTooltip}> - <FontAwesomeIconWrapper icon={'fa-solid fa-medal'} className="h-4 w-4" /> - </Tooltip> - </div> - ) - } - - const iconDef = findIconDefinition({ - prefix: 'fas', - iconName: cssClass.split(' ').pop().replace('fa-', '') as IconName, - }) - - return ( - <div className="inline-flex items-center"> - <Tooltip content={name} isDisabled={!showTooltip}> - <FontAwesomeIconWrapper - icon={iconDef ? `fa-solid ${cssClass}` : 'fa-solid fa-question'} - className="h-4 w-4" - /> - </Tooltip> - </div> - ) -} +const Badges = ({ name, cssClass, showTooltip = true }: BadgeProps) => { + const resolvedIcon = + typeof cssClass === 'string' && cssClass.trim().length > 0 ? cssClass.trim() : 'fa-solid fa-medal' + return ( + <div className="inline-flex items-center"> + <Tooltip content={name} isDisabled={!showTooltip}> + <FontAwesomeIconWrapper icon={resolvedIcon} className="h-4 w-4" /> + </Tooltip> + </div> + ) +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/Badges.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (3)
frontend/src/components/Badges.tsx (3)
12-20: Fallback path looks good.Gracefully falls back to a solid medal when cssClass is absent or invalid.
22-25: Remove svg-core lookup; it’s unsafe here and breaks brands/custom icons.
findIconDefinitionassumes icons are registered vialibrary.add(...)and defaults to'fas', so it will (a) throw/not find icons when packs aren’t registered and (b) mis-handlefa-regular/fa-brandsand any custom/local icons — violating the PR objective to support custom/OWASP icons. Prefer passing the provided class straight to the wrapper and only fall back when cssClass is absent.Apply this diff:
- const iconDef = findIconDefinition({ - prefix: 'fas', - iconName: cssClass.split(' ').pop().replace('fa-', '') as IconName, - })Based on learnings
27-35: Don’t hard-code ‘fa-solid’; render the provided class as-is.Prepending
fa-solidcreates invalid combos likefa-solid fa-brands fa-github. Also, theiconDefcheck is unnecessary after removing the lookup; just passcssClass.Apply this diff:
- <Tooltip content={name} isDisabled={!showTooltip}> - <FontAwesomeIconWrapper - icon={iconDef ? `fa-solid ${cssClass}` : 'fa-solid fa-question'} - className="h-4 w-4" - /> - </Tooltip> + <Tooltip content={name} isDisabled={!showTooltip}> + <FontAwesomeIconWrapper icon={cssClass} className="h-4 w-4" /> + </Tooltip>
|
@kasya done with the changes and the heatmap is aligned with its skeleton |
|
@arkid15r please take a look here |
Proposed change
Resolves #1765
created the frontend implementation for the user badges for user card with a count and all the badges on the user detail page with tooltip for its name and also the badges are sorted properly.
I have also attached the demo badges screenshots and the badges in the summary section will changes according to the css_class coming from backend.
Checklist
make check-testlocally; all checks and tests passed.