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 badge support end-to-end: backend exposes UserNode.badge_count and refines badges resolver; indexing gains idx_badge_count. Badge model gains css_class choices and migration. Frontend introduces Badges component, updates UserCard and member pages, extends GraphQL queries and types, supplies icon map, and adds extensive unit tests. Also pins @swc/core. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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: 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 |
|
@arkid15r please take a look here |
There was a problem hiding this comment.
Your approach with handling icons on the frontend is very limited.
At the moment, anything can be sent from the backend, and even if you add dynamic imports for these icons on the frontend, maintaining good performance will still be difficult.
I recommend you changing it to:
- Limit what is saved on the
Badgemodel on the backend forcss_classfield withchoicesand add validation (5-10 icons at most) - Import these icons on the frontend and add to library in
utils/data.tsso we are always ready to show them.
This is the simplest solution.
|
@Piyushrathoree just wanted to check if you will be working on this updates further? I'm just not sure after tour response. |
i'll be working on this and fix all the things today |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/server/queries/userQueries.ts (1)
12-18: Inconsistent field ordering between queries.The badges field selection has different ordering in GET_LEADER_DATA (id, name, description, cssClass) versus GET_USER_DATA (cssClass, description, id, name). While functionally equivalent, consistent ordering improves maintainability.
Apply this diff to standardize the field order in GET_USER_DATA:
badges { - cssClass - description id name + description + cssClass }Also applies to: 90-96
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/userQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (4)
frontend/__tests__/unit/pages/UserDetails.test.tsx(3 hunks)frontend/src/app/members/[memberKey]/page.tsx(3 hunks)frontend/src/server/queries/userQueries.ts(3 hunks)frontend/src/types/user.ts(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)
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
frontend/__tests__/unit/data/mockUserDetails.ts (1)
mockUserDetailsData(1-83)
frontend/src/types/user.ts (1)
frontend/src/types/badge.ts (1)
Badge(1-7)
🔇 Additional comments (6)
frontend/src/types/user.ts (1)
1-1: LGTM! Type extensions align with badge feature.The Badge import and optional User type fields (badges and badgeCount) are correctly defined and consistent with the GraphQL schema extensions.
Also applies to: 14-15
frontend/__tests__/unit/pages/UserDetails.test.tsx (4)
18-30: LGTM! FontAwesomeIcon mock correctly handles different icon prop formats.The mock properly extracts the icon name from both array format (e.g., ['fas', 'star']) and object format (e.g., { iconName: 'star' }), making it suitable for testing various FontAwesome icon usages.
32-56: LGTM! Badges component mock provides clear test hooks.The MockBadges component correctly:
- Accepts the expected props (name, cssClass, showTooltip)
- Creates predictable test IDs by slugifying badge names
- Exposes data attributes for assertion purposes
- Renders a nested icon element for comprehensive testing
69-69: LGTM! Route parameter updated to match current naming.The change from
userKeytomemberKeyaligns with the route parameter rename to[memberKey].Note: A past review comment indicated that other test files (Users.test.tsx, Projects.test.tsx) also need similar updates, but those files are not in the current review scope.
539-735: LGTM! Comprehensive badge test coverage.The Badge Display Tests suite thoroughly covers:
- Basic rendering when badges are present
- Props validation (cssClass, showTooltip)
- Graceful handling of empty/undefined badges
- Fallback behavior for missing cssClass
- Edge cases (special characters, long names)
This provides strong confidence in the badge display functionality.
frontend/src/server/queries/userQueries.ts (1)
12-18: No need to fetchweight—it’s unused on the frontend. No occurrences of.weightor"weight"were found in any.ts/.tsxfiles, so including it in the query is unnecessary.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/nest/migrations/0007_alter_badge_css_class.py(1 hunks)backend/apps/nest/models/badge.py(2 hunks)frontend/__tests__/unit/components/Badges.test.tsx(1 hunks)frontend/src/app/members/[memberKey]/page.tsx(3 hunks)frontend/src/components/Badges.tsx(1 hunks)frontend/src/utils/data.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/Badges.tsx
- frontend/src/app/members/[memberKey]/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/Badges.test.tsx (1)
frontend/src/utils/data.ts (1)
BADGE_CLASS_MAP(77-84)
🪛 GitHub Actions: Run CI/CD
frontend/src/utils/data.ts
[error] 83-83: Object Literal Property name bug_slash must match one of the following formats: camelCase, UPPER_CASE (naming-convention)
frontend/__tests__/unit/components/Badges.test.tsx
[warning] 21-21: Unexpected any. Specify a different type. (no-explicit-any)
[warning] 34-34: Unexpected any. Specify a different type. (no-explicit-any)
🔇 Additional comments (6)
frontend/src/utils/data.ts (1)
35-50: LGTM!The icon imports and library registrations are correctly implemented. All newly imported badge icons are properly added to the FontAwesome library.
backend/apps/nest/migrations/0007_alter_badge_css_class.py (1)
1-29: LGTM!The migration correctly adds choices to the
css_classfield with appropriate defaults. The choices align with theBadgeCssClassenum defined in the model.backend/apps/nest/models/badge.py (2)
13-19: LGTM!The
BadgeCssClassenum is correctly implemented using Django'sTextChoices. The snake_case database values follow Django conventions, and display names are appropriately formatted.
26-31: LGTM!The
css_classfield is correctly updated to use theBadgeCssClasschoices with a sensible default value. This change improves data integrity by constraining values to the defined set.frontend/__tests__/unit/components/Badges.test.tsx (2)
5-16: Improved mock implementation addresses past concerns.This mock setup is a significant improvement over the previous overmocked version. By importing the actual
BADGE_CLASS_MAPand using it to determine which icons are registered, the tests now more closely reflect production behavior.
72-104: LGTM!The test cases provide good coverage of the component's core functionality, including valid/invalid icon rendering, fallback behavior, and tooltip visibility control.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/app/members/[memberKey]/page.tsx (1)
206-206: Remove unnecessary.slice()call.The
.slice()call creates a copy of the array before mapping, but it's not needed here since the operation is read-only and doesn't mutate the original array. This adds unnecessary overhead.Apply this diff:
- {user.badges.slice().map((badge: Badge) => ( + {user.badges.map((badge: Badge) => (
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/app/members/[memberKey]/page.tsx(3 hunks)frontend/src/utils/data.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/members/[memberKey]/page.tsx (1)
frontend/src/types/badge.ts (1)
Badge(1-7)
⏰ 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). (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
frontend/src/utils/data.ts (1)
77-84: VerifybugSlashicon mapping and badge key conversion
- The key
bugSlashmaps tofaBug; update tofaBugSlashif that was intended.- Confirm that backend
bug_slashidentifiers are converted to camelCase before lookup in this map.frontend/src/app/members/[memberKey]/page.tsx (1)
208-212: Badge component prop signature updated
TheBadgePropstype in frontend/src/components/Badges.tsx is defined astype BadgeProps = { name: string cssClass: string showTooltip?: boolean }It no longer accepts
descriptionorweight; passing onlyname,cssClass, andshowTooltipis correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
backend/apps/github/models/mixins/user.py (1)
185-189: LGTM! Consistent with existing index properties.The implementation correctly filters active badges and returns the count. The approach is consistent with other count properties in this mixin (e.g.,
idx_issues_count,idx_releases_count).Optional: If indexing performance becomes a concern, consider using
select_related/prefetch_relatedat the indexing call site to minimize database queries, though this optimization would apply to allidx_properties collectively rather than this one in isolation.frontend/src/components/Badges.tsx (1)
18-21: Simplify icon resolution logic.The function is clear, but you can streamline it slightly.
Consider:
const resolveIcon = (cssClass: string) => { - const normalizedClass = normalizeCssClass(cssClass) - return BADGE_CLASS_MAP[normalizedClass] ?? DEFAULT_ICON + const normalized = normalizeCssClass(cssClass) + return BADGE_CLASS_MAP[normalized] || DEFAULT_ICON }frontend/__tests__/unit/components/Badges.test.tsx (2)
8-19: Replace implicitanywith proper TypeScript types.The
getNamefunction parameter uses an implicitanytype. While the logic is sound, adding explicit types improves type safety and maintainability.Apply this diff:
+import type { IconDefinition } from '@fortawesome/fontawesome-svg-core' + jest.mock('wrappers/FontAwesomeIconWrapper', () => { const RealWrapper = jest.requireActual('wrappers/FontAwesomeIconWrapper').default - const getName = (icon) => { + const getName = (icon: IconDefinition | string | [string, string] | undefined) => { if (!icon) return 'medal' if (typeof icon === 'string') { const m = icon.match(/fa-([a-z0-9-]+)$/i) if (m) return m[1] const last = icon.trim().split(/\s+/).pop() || '' return last.replace(/^fa-/, '') || 'medal' } if (Array.isArray(icon) && icon.length >= 2) return String(icon[1]) - if (icon && typeof icon === 'object') return icon.iconName || String(icon[1] ?? 'medal') + if (icon && typeof icon === 'object' && 'iconName' in icon) { + return icon.iconName || 'medal' + } return 'medal' }
21-28: Replace implicitanytype in mock function.The
propsparameter uses an implicitanytype.Apply this diff:
- return function MockFontAwesomeIconWrapper(props) { + return function MockFontAwesomeIconWrapper(props: { + icon: IconDefinition | string | [string, string] | undefined + [key: string]: unknown + }) { const name = getName(props.icon) return ( <div data-testid={`icon-${name}`}> <RealWrapper {...props} /> </div> ) }frontend/src/server/queries/userQueries.ts (1)
12-19: Consider removing unusedweightfield from badges query.The
weightfield is queried in bothGET_LEADER_DATAandGET_USER_DATA, but theBadgescomponent (frontend/src/components/Badges.tsx) doesn't consume it. It only appears in test mock data. Unless you plan to use it for sorting or display logic in the near future, removing it reduces the GraphQL payload size.If you intend to keep it for future use (e.g., client-side badge sorting), document that intent in a comment. Otherwise, apply:
badges { id name description cssClass - weight }Also applies to: 91-98
📜 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 (11)
backend/apps/core/utils/index.py(1 hunks)backend/apps/github/index/registry/user.py(1 hunks)backend/apps/github/index/search/user.py(1 hunks)backend/apps/github/models/mixins/user.py(1 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)frontend/src/server/queries/userQueries.ts(3 hunks)frontend/src/utils/data.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/app/members/[memberKey]/page.tsx
- frontend/src/utils/data.ts
- frontend/src/app/members/page.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/Badges.tsx (1)
frontend/src/utils/data.ts (1)
BADGE_CLASS_MAP(78-85)
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
frontend/__tests__/unit/data/mockUserDetails.ts (1)
mockUserDetailsData(1-83)
🔇 Additional comments (9)
backend/apps/core/utils/index.py (1)
199-199: LGTM! Proper alphabetical placement.The addition of
idx_badge_countto the users index attributes is correct and maintains alphabetical ordering.backend/apps/github/index/search/user.py (1)
35-35: LGTM! Consistent alphabetical ordering.The addition of
idx_badge_countto the default attributes is correct and properly positioned alphabetically.backend/apps/github/index/registry/user.py (1)
17-17: LGTM! Completes the indexing infrastructure.The addition of
idx_badge_countto the index fields properly integrates the new badge count capability into the Algolia index configuration, maintaining alphabetical order and completing the changes across the indexing stack (model → registry → search → utils).frontend/src/components/Badges.tsx (1)
23-33: Implementation addresses past review concerns effectively.The component now:
- Uses
BADGE_CLASS_MAPfor consistent icon resolution- Leverages
Tooltip'sisDisabledprop to avoid code duplication- Falls back gracefully to the medal icon
- Passes the resolved
IconDefinitiondirectly toFontAwesomeIconWrapperThis is a significant improvement over earlier iterations that had FA lookup issues and redundant conditional rendering.
frontend/__tests__/unit/components/Badges.test.tsx (1)
93-120: Backend enum icon tests are comprehensive.The test suite correctly validates:
- All backend enum values (including
bug_slashwith snake_case)- Snake_case to camelCase transformation
- Direct camelCase input handling
This addresses the prior concern about inconsistent naming between backend and frontend.
frontend/src/server/queries/userQueries.ts (1)
106-116: GraphQL query additions align with badge requirements.Adding
badgeCountand the fullbadgesarray with necessary fields (id,name,description,cssClass) enables the frontend to render user badges per the PR objectives.frontend/__tests__/unit/pages/UserDetails.test.tsx (3)
18-30: FontAwesomeIcon mock properly typed and flexible.The updated mock correctly handles multiple icon format variations (array, object with iconName) and is now properly typed, addressing the TypeScript safety concern from earlier reviews.
32-56: MockBadges implementation exposes testable attributes.The mock component:
- Renders with a predictable
data-testidbased on the badge name- Exposes
cssClassandshowTooltipfor verification- Nests the icon element with its own testid
This structure enables comprehensive assertions in the badge display tests below.
539-735: Badge display tests are thorough and cover edge cases.The test suite validates:
- Basic badge rendering and props
- Empty/undefined badge arrays
- Fallback behavior for missing/empty
cssClass- Special characters and long names in badge names
- Conditional rendering based on badge presence
This comprehensive coverage should catch rendering issues that earlier mocked tests missed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/app/members/[memberKey]/page.tsx (1)
210-210: Remove unnecessary.slice()call.The
.slice()creates an array copy but the code doesn't mutate the array. Since the backend provides badges in the correct order (as discussed in previous comments), this allocation is unnecessary.Apply this diff:
- {user.badges.slice().map((badge: Badge) => ( + {user.badges.map((badge: Badge) => (
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/userQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (12)
backend/apps/core/utils/index.py(1 hunks)backend/apps/github/index/registry/user.py(1 hunks)backend/apps/github/index/search/user.py(1 hunks)backend/apps/github/models/mixins/user.py(1 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)frontend/src/server/queries/userQueries.ts(3 hunks)frontend/src/types/card.ts(2 hunks)frontend/src/utils/data.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/tests/unit/components/Badges.test.tsx
- frontend/src/server/queries/userQueries.ts
- frontend/src/app/members/page.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/types/card.ts (1)
frontend/src/types/badge.ts (1)
Badge(1-7)
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
frontend/__tests__/unit/data/mockUserDetails.ts (1)
mockUserDetailsData(1-83)
frontend/src/components/Badges.tsx (1)
frontend/src/utils/data.ts (1)
BADGE_CLASS_MAP(78-85)
frontend/src/app/members/[memberKey]/page.tsx (1)
frontend/src/types/badge.ts (1)
Badge(1-7)
🪛 GitHub Actions: Run CI/CD
frontend/src/utils/data.ts
[error] 72-72: Unstaged changes detected. Run make check and use git add to address it.
🔇 Additional comments (10)
backend/apps/github/models/mixins/user.py (1)
186-189: LGTM!The
idx_badge_countproperty correctly filters active badges and returns the count. The implementation is consistent with other indexing properties in this mixin.backend/apps/github/index/search/user.py (1)
35-35: LGTM!The
idx_badge_countattribute is correctly added to the default retrieval list in alphabetical order, consistent with the indexing changes across the codebase.backend/apps/github/index/registry/user.py (1)
17-17: LGTM!The
idx_badge_countfield is properly registered in theUserIndexfields tuple, maintaining alphabetical order and consistency with the model property.backend/apps/core/utils/index.py (1)
199-199: LGTM!The
idx_badge_countattribute is correctly included in theattributesToRetrievelist for the "users" index, maintaining alphabetical order and consistency with the broader indexing changes.frontend/src/types/card.ts (1)
3-3: LGTM!The
Badgetype import and the new optionalbadgesandbadgeCountfields onUserCardPropsare correctly typed and appropriately optional, allowing components to render badge data when available.Also applies to: 82-83
frontend/src/utils/data.ts (1)
1-1: LGTM!The badge icon imports, library registration, and
BADGE_CLASS_MAPare correctly implemented. The naming convention issue withbugSlash(previouslybug_slash) has been resolved, and all icons are properly imported and added to the FontAwesome library.Note: The pipeline failure regarding unstaged changes should be resolved by running
make checkand staging the formatted changes, as indicated by the CI error.Also applies to: 35-40, 47-51, 78-85
frontend/src/app/members/[memberKey]/page.tsx (1)
208-220: Consider displaying the badge count.The
badgeCountfield is available from the GraphQL query (as seen inUserCardProps), but it's not displayed in the UI. Showing a count (e.g., "5 badges" or a numeric indicator) could enhance the user experience, especially if there are many badges.Should the badge count be displayed in the user detail header, or is rendering all badges sufficient for this view? On the members list page (where space is limited), a count might be more appropriate than rendering all badges.
frontend/src/components/Badges.tsx (1)
23-33: LGTM!The
Badgescomponent implementation is clean and correct:
- Uses
Tooltip'sisDisabledprop to conditionally show tooltips without code duplication (addressing previous feedback).- Resolves icons via
BADGE_CLASS_MAPwith a sensible fallback toDEFAULT_ICON.- Passes
IconDefinitionobjects directly toFontAwesomeIconWrapper, which is the correct pattern.- Includes
data-testidfor testing.frontend/__tests__/unit/pages/UserDetails.test.tsx (2)
18-29: LGTM - FontAwesomeIcon mock handles both icon formats correctly.The updated mock properly extracts the icon name from either array format
['prefix', 'iconName']or object format{ iconName: 'name' }, making it flexible for different FontAwesome usage patterns in the codebase.
32-56: Mock props align with real Badges componentThe mock’s
name,cssClass, and optionalshowTooltipprops exactly match theBadgePropsinfrontend/src/components/Badges.tsx. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)
131-190: LGTM!The test correctly verifies that the badges resolver configures the queryset with proper ordering (
order_by("badge__weight", "badge__name")) and returns badges in the expected sequence.Optional: Consider removing unused mock attributes.
The
weightandnameattributes on the mock badges (lines 135-148) are never accessed since this unit test only verifies the ORM method calls, not the actual sorting logic. While they serve as documentation, removing them would clarify that this test validates the queryset configuration rather than the sorting behavior itself.If you prefer to keep them for documentation purposes, that's also fine.
📜 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/core/utils/match_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/user_test.py(3 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/github/api/internal/nodes/user.py
🧰 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(19-20)backend/apps/github/api/internal/nodes/user.py (3)
UserNode(28-67)badge_count(40-42)badges(32-37)
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
frontend/__tests__/unit/data/mockUserDetails.ts (1)
mockUserDetailsData(1-83)
⏰ 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 (9)
backend/tests/apps/core/utils/match_test.py (1)
117-117: LGTM! Test correctly updated to reflect implementation changes.The addition of
"idx_badge_count"to theattributesToRetrievelist is correctly positioned in alphabetical order and aligns with the implementation changes that introduce badge count indexing for users.backend/tests/apps/github/api/internal/nodes/user_test.py (5)
8-8: LGTM!The BadgeNode import is correctly added and used appropriately in the test methods below.
21-23: LGTM!The addition of "badge_count" to the expected field names correctly reflects the new public API field on UserNode.
86-97: LGTM!The test correctly verifies the badge_count resolver implementation, properly mocking the queryset chain (
values("badge_id").distinct().count()) and confirming all method calls are made as expected.
99-111: LGTM!The test correctly verifies the badges resolver behavior with an empty badges list, ensuring the queryset chain (
select_related("badge").order_by("badge__weight", "badge__name")) is properly configured even when no badges are present.
113-129: LGTM!The test correctly verifies the badges resolver with a single badge, properly using
spec=BadgeNodefor type safety and confirming the queryset chain is configured correctly.frontend/__tests__/unit/pages/UserDetails.test.tsx (3)
18-29: LGTM — FontAwesomeIcon mock handles both icon formats correctly.The updated mock appropriately derives
iconNamefrom either array or object format and renders the expected test elements.
32-56: LGTM — Badges mock component is well-structured for testing.The mock correctly exposes badge properties via data attributes and test IDs, enabling thorough verification of badge rendering behavior.
540-734: Excellent edge case coverage for badge rendering.The test suite comprehensively covers:
- Presence/absence of badges
- Undefined/empty badge arrays
- Fallback cssClass behavior
- Special characters and long names
|
There was a problem hiding this comment.
hey @Piyushrathoree !
This was taking way too long to address all the comments, so I ended up pushing changes myself so that we can release this today.
Thank you for working on this.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/apps/github/api/internal/nodes/user.py (1)
31-34: Eliminate code duplication by reusingidx_badge_count.The
badge_countfield duplicates the logic fromidx_badge_countin theUserIndexMixin. Consider reusing the existing property for consistency and maintainability.Apply this diff:
@strawberry.field def badge_count(self) -> int: """Resolve badge count.""" - return self.user_badges.filter(is_active=True).count() + return self.idx_badge_count
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/src/types/__generated__/userQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (11)
backend/apps/github/api/internal/nodes/user.py(1 hunks)backend/apps/github/models/mixins/user.py(1 hunks)backend/apps/nest/models/badge.py(2 hunks)backend/tests/apps/github/api/internal/nodes/user_test.py(3 hunks)frontend/package.json(1 hunks)frontend/src/app/members/page.tsx(1 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)frontend/src/utils/data.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/types/badge.ts
- backend/apps/nest/models/badge.py
- frontend/src/server/queries/userQueries.ts
- frontend/src/types/card.ts
- frontend/src/app/members/page.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/types/user.ts (2)
backend/apps/nest/models/badge.py (1)
Badge(10-50)frontend/src/types/badge.ts (1)
Badge(1-7)
backend/tests/apps/github/api/internal/nodes/user_test.py (3)
frontend/src/types/__generated__/graphql.ts (2)
BadgeNode(40-47)UserNode(877-899)backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(19-20)backend/apps/github/api/internal/nodes/user.py (3)
UserNode(28-76)badge_count(32-34)badges(37-51)
backend/apps/github/api/internal/nodes/user.py (2)
frontend/src/types/__generated__/graphql.ts (1)
BadgeNode(40-47)backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(19-20)
⏰ 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 unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (10)
frontend/package.json (1)
80-80: Why downgrade and pin@swc/core?This change downgrades from the previously allowed
^1.13.20(which includes 1.13.20 and newer patch releases) to a pinned1.13.19, dropping fixes and security patches published in 1.13.20+. Please confirm the regression you’re avoiding and consider pinning to at least1.13.20(or whatever patch contains the required fix) so we don’t lock in an older, potentially vulnerable build.backend/apps/github/models/mixins/user.py (1)
27-30: LGTM!The
idx_badge_countproperty correctly filters active badges and returns the count. The implementation is consistent with other index properties in this mixin.backend/apps/github/api/internal/nodes/user.py (1)
36-51: LGTM!The
badgesresolver correctly:
- Filters for active badges
- Uses
select_related("badge")to avoid N+1 queries- Orders by weight and name
- Returns the badge objects
backend/tests/apps/github/api/internal/nodes/user_test.py (6)
8-8: LGTM!Import of
BadgeNodeis necessary for the new badge-related tests.
23-23: LGTM!Correctly adds
badge_countto the expected field names, reflecting the new field onUserNode.
86-96: LGTM!The test correctly validates that
badge_countcallsfilter(is_active=True).count()on theuser_badgesqueryset and returns the expected count.
98-111: LGTM!The test properly verifies the empty badges scenario, ensuring the queryset chain (
filter→select_related→order_by) is called with correct arguments and returns an empty list.
113-130: LGTM!The test correctly validates single badge resolution, verifying both the queryset chain and the returned badge object.
132-192: LGTM!This comprehensive test validates the sorting behavior with multiple badges of varying weights and names. The test setup and assertions correctly verify:
- Badges are ordered by weight (ascending), then by name (ascending)
- The queryset methods are called with the correct arguments
- The returned list matches the expected order
frontend/src/types/user.ts (1)
13-14: User badge fields wired correctlyOptional
badgeCount/badgesalign with backend additions and keep the User type backward-compatible. Nice work.
3 participants
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.