Impove chapter/project leaders presentation #1772
Conversation
Summary by CodeRabbit
WalkthroughThis change refactors the display of leaders across the application. The leader presentation logic is extracted into a new reusable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Complexity label: Moderate Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope or unrelated code changes detected. Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
backend/apps/owasp/graphql/nodes/common.py(1 hunks)frontend/src/app/about/page.tsx(2 hunks)frontend/src/app/projects/[projectKey]/page.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(4 hunks)frontend/src/components/LeadersListBlock.tsx(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/leaders.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
🧠 Learnings (7)
frontend/src/types/card.ts (2)
Learnt from: ahmedxgouda
PR: #1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: healthMetricsData.length > 0. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: #1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: healthMetricsData && healthMetricsData.length > 0. This makes accessing data[0] safe within the HealthMetrics component.
backend/apps/owasp/graphql/nodes/common.py (2)
Learnt from: Rajgupta36
PR: #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.
Learnt from: Rajgupta36
PR: #1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
frontend/src/app/projects/[projectKey]/page.tsx (2)
Learnt from: ahmedxgouda
PR: #1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: healthMetricsData.length > 0. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: #1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: healthMetricsData && healthMetricsData.length > 0. This makes accessing data[0] safe within the HealthMetrics component.
frontend/src/types/leaders.ts (1)
Learnt from: ahmedxgouda
PR: #1714
File: frontend/src/components/ProjectTypeDashboardCard.tsx:8-12
Timestamp: 2025-07-08T17:07:50.988Z
Learning: In the OWASP/Nest project, union types for component props are not necessary when they would require creating separate type definitions. The project prefers inline prop type definitions even for props with specific string values, maintaining consistency with the single-use component prop pattern.
frontend/src/components/CardDetailsPage.tsx (2)
Learnt from: ahmedxgouda
PR: #1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: healthMetricsData.length > 0. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: #1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: healthMetricsData && healthMetricsData.length > 0. This makes accessing data[0] safe within the HealthMetrics component.
frontend/src/components/LeadersListBlock.tsx (1)
Learnt from: Rajgupta36
PR: #1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
frontend/src/app/about/page.tsx (3)
Learnt from: Rajgupta36
PR: #1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Learnt from: Rajgupta36
PR: #1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Learnt from: Rajgupta36
PR: #1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is (session as ExtendedSession)?.user?.login.
🧰 Additional context used
🧠 Learnings (7)
frontend/src/types/card.ts (2)
Learnt from: ahmedxgouda
PR: #1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: healthMetricsData.length > 0. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: #1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: healthMetricsData && healthMetricsData.length > 0. This makes accessing data[0] safe within the HealthMetrics component.
backend/apps/owasp/graphql/nodes/common.py (2)
Learnt from: Rajgupta36
PR: #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.
Learnt from: Rajgupta36
PR: #1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
frontend/src/app/projects/[projectKey]/page.tsx (2)
Learnt from: ahmedxgouda
PR: #1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: healthMetricsData.length > 0. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: #1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: healthMetricsData && healthMetricsData.length > 0. This makes accessing data[0] safe within the HealthMetrics component.
frontend/src/types/leaders.ts (1)
Learnt from: ahmedxgouda
PR: #1714
File: frontend/src/components/ProjectTypeDashboardCard.tsx:8-12
Timestamp: 2025-07-08T17:07:50.988Z
Learning: In the OWASP/Nest project, union types for component props are not necessary when they would require creating separate type definitions. The project prefers inline prop type definitions even for props with specific string values, maintaining consistency with the single-use component prop pattern.
frontend/src/components/CardDetailsPage.tsx (2)
Learnt from: ahmedxgouda
PR: #1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: healthMetricsData.length > 0. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: #1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: healthMetricsData && healthMetricsData.length > 0. This makes accessing data[0] safe within the HealthMetrics component.
frontend/src/components/LeadersListBlock.tsx (1)
Learnt from: Rajgupta36
PR: #1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
frontend/src/app/about/page.tsx (3)
Learnt from: Rajgupta36
PR: #1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Learnt from: Rajgupta36
PR: #1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Learnt from: Rajgupta36
PR: #1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is (session as ExtendedSession)?.user?.login.
🔇 Additional comments (16)
backend/apps/owasp/graphql/nodes/common.py (2)
15-15: Consider consistency between data sources for leaders.The new
leaders_loginsmethod usesself.leaders.all()while the existingleadersmethod usesself.idx_leaders. Ensure this difference in data sources is intentional and that both provide consistent leader information.
12-15: leaders_logins resolver: login attribute confirmed
Theloginfield is defined on all leader models inbackend/apps/github/models/common.py, so each object returned byself.leaders.all()will always have a.login. No additional error handling for missingloginis necessary.frontend/src/types/card.ts (1)
56-56: LGTM - Type definition is correct and consistent.The addition of the optional
leadersLoginsproperty withstring[]type aligns perfectly with the backend GraphQL resolver and maintains backward compatibility.frontend/src/app/projects/[projectKey]/page.tsx (1)
106-106: ✅ GraphQL query includes leadersLoginsI’ve verified that
GET_PROJECT_DATAinserver/queries/projectQueries.tsincludes theleadersLoginsfield, soproject.leadersLoginswill be populated correctly. No further action required.frontend/src/types/leaders.ts (1)
5-8: Ensure mapping from leadersLogins array to LeadersListBlockPropsI wasn’t able to find where the backend’s
leadersLogins: string[]is transformed into theRecord<string, string>thatLeadersListBlockexpects. Please verify that:
- In the data-loading layer (page/component or hook that fetches
leadersLogins), you convert the array into an object mapping each username to its certification string.- The parent component always passes a correctly shaped
leaders: LeadersListBlockPropsintoLeadersListBlock.frontend/src/server/queries/projectQueries.ts (1)
13-13: LGTM! Clean addition of leadersLogins field.The new
leadersLoginsfield is appropriately placed in the GraphQL query and aligns with the backend changes to support the enhanced leaders presentation.frontend/src/components/CardDetailsPage.tsx (3)
14-14: LGTM! Clean imports for the new LeadersListBlock functionality.The imports are correctly added to support the new leaders presentation feature.
Also applies to: 23-23
54-54: LGTM! Optional prop maintains backward compatibility.The
leadersLoginsprop is correctly typed as optional, ensuring existing usage of DetailsCard continues to work.
203-211: LGTM! Clean conditional rendering with proper data transformation.The conditional rendering follows the existing pattern in the component, and the reduce function correctly transforms the
leadersLoginsarray into the expected object structure for theLeadersListBlockcomponent.frontend/src/app/about/page.tsx (2)
26-26: LGTM! Clean import for the reusable LeadersListBlock component.The import supports the refactoring to centralize leader display logic.
111-113: LGTM! Excellent refactoring to use the reusable LeadersListBlock.This change simplifies the code by removing the inline
LeaderDatacomponent logic and consolidating it into the reusableLeadersListBlockcomponent. The props are correctly passed and the conditional rendering pattern is maintained.frontend/src/components/LeadersListBlock.tsx (5)
1-12: LGTM! Clean imports and proper client-side directive.All necessary imports are included and the 'use client' directive is correctly placed for this interactive component.
13-21: LGTM! Well-defined component interface with proper typing.The component props are clearly typed with appropriate defaults. The optional icon and label parameters provide good flexibility.
22-56: LGTM! Robust LeaderData component with proper error handling.The nested LeaderData component correctly handles:
- Loading states with user-friendly messages
- Error states with descriptive feedback
- Missing data scenarios
- Proper navigation using Next.js router
- Clean UserCard integration
The implementation follows React best practices and provides a good user experience.
58-69: LGTM! Clean responsive layout and component structure.The SecondaryCard wrapper with AnchorTitle provides consistent styling, and the responsive flexbox layout (column on mobile, row on desktop) ensures good user experience across devices.
51-51: Leader Username Mapping VerifiedThe
leadersobject infrontend/src/app/about/page.tsxdefines keysarkid15r,kasya, andmamicidal, which correspond exactly to theuser.loginvalues used inLeadersListBlock.tsx(line 51). No mismatches were found—no further changes needed.
|
almost done with the PR, just need to add tests and fix some sonar issues. |
|
adding tests. |
|
I kept the UI Layout exactly the same as stated in the parent issue. However I think I can improve this by changing the Button width from 
|
|
@rudransh-shrivastava of course! Feel free to update the width to accommodate longer titles/names. |
| @strawberry.field | ||
| def leaders_logins(self) -> list[str]: | ||
| """Resolve leaders logins.""" | ||
| return [leader.login for leader in self.leaders.all()] |
There was a problem hiding this comment.
@rudransh-shrivastava were you able to see any Leaders data on the frontend? Checking this, and I don't think this resolver returns anything - it's just an empty list all the time. And that's because there's no self.leaders.all()
There was a problem hiding this comment.
Also, feel free to use http://localhost:8000/graphql/ to see data that is returned by graphQL request while working on updating queries - this is super helpful!
There was a problem hiding this comment.
Yes @kasya , you're right that there are no leaders.
There are two fields in the OWASP database model
since leaders maps to github.User, I was using this field to access leader's logins/usernames.
The leaders_raw field is a list of strings like ["Rudransh Shrivastava"].
leaders for projects or chapters do not have any data in the database -- but for leaders_raw there is.
If i take a random project for example:
This is the leaders_raw field.
This is the leaders field - which is empty.
I'm not sure why leaders is empty. Initially I assumed it was an issue locally. I was able to set leaders manually and test out things.
rudransh-shrivastava
force-pushed
the
feature/improve-leaders-presentation
branch
from
ae244d5 to
a561310
Compare
rudransh-shrivastava
force-pushed
the
feature/improve-leaders-presentation
branch
from
a561310 to
a0edae1
Compare
|
|
Hey @rudransh-shrivastava ! Thank you for working on this! I think we should move it to DRAFT state until it's ready to get back to. 👌🏼 |
|
Hey, that's okay, please let me know when the changes are done, I'll work on this again and fix things that may have broken |
Resolves #1431