Skip to content

Improve chapters/project leaders presentation #1729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Improve chapters/project leaders presentation #1729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions backend/apps/owasp/graphql/nodes/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,18 @@
import strawberry

from apps.github.graphql.nodes.repository_contributor import RepositoryContributorNode
from apps.github.graphql.nodes.user import UserNode


@strawberry.type
class GenericEntityNode:
"""Base node class for OWASP entities with common fields and resolvers."""

@strawberry.field
def leaders_temp(self) -> list[UserNode]:
"""Resolve leaders logins."""
return self.leaders.all()
Comment on lines +13 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider improving method naming and documentation.

The implementation correctly exposes User objects via GraphQL, but consider the following improvements:

  1. The method name leaders_temp suggests this is temporary. Based on the PR objectives, consider planning for the eventual rename.
  2. The docstring says "Resolve leaders logins" but it actually returns User objects, not just logins.
-    def leaders_temp(self) -> list[UserNode]:
-        """Resolve leaders logins."""
+    def leaders_temp(self) -> list[UserNode]:
+        """Resolve leaders as User objects."""
📝 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.

Suggested change
@strawberry.field
def leaders_temp(self) -> list[UserNode]:
"""Resolve leaders logins."""
return self.leaders.all()
@strawberry.field
def leaders_temp(self) -> list[UserNode]:
"""Resolve leaders as User objects."""
return self.leaders.all()
🤖 Prompt for AI Agents 
In backend/apps/owasp/graphql/nodes/common.py around lines 13 to 16, rename the
method from leaders_temp to a more descriptive and permanent name like leaders
to reflect its purpose accurately. Update the docstring to clearly state that
the method returns a list of User objects representing leaders, not just their
logins, ensuring the documentation matches the method's functionality.


@strawberry.field
def leaders(self) -> list[str]:
"""Resolve leaders."""
Expand Down
54 changes: 4 additions & 50 deletions frontend/src/app/about/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,20 @@ import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'
import { Tooltip } from '@heroui/tooltip'
import Image from 'next/image'
import Link from 'next/link'
import { useRouter } from 'next/navigation'
import { useEffect, useState } from 'react'
import FontAwesomeIconWrapper from 'wrappers/FontAwesomeIconWrapper'
import { ErrorDisplay, handleAppError } from 'app/global-error'
import { GET_PROJECT_METADATA, GET_TOP_CONTRIBUTORS } from 'server/queries/projectQueries'
import { GET_LEADER_DATA } from 'server/queries/userQueries'
import type { Contributor } from 'types/contributor'
import type { Project } from 'types/project'
import type { User } from 'types/user'
import { aboutText, technologies } from 'utils/aboutData'
import { capitalize } from 'utils/capitalize'
import AnchorTitle from 'components/AnchorTitle'
import AnimatedCounter from 'components/AnimatedCounter'
import LeadersListBlock from 'components/LeadersListBlock'
import LoadingSpinner from 'components/LoadingSpinner'
import Markdown from 'components/MarkdownWrapper'
import SecondaryCard from 'components/SecondaryCard'
import TopContributorsList from 'components/TopContributorsList'
import UserCard from 'components/UserCard'

const leaders = {
arkid15r: 'CCSP, CISSP, CSSLP',
Expand Down Expand Up @@ -112,15 +108,9 @@ const About = () => {
))}
</SecondaryCard>

<SecondaryCard icon={faArrowUpRightFromSquare} title={<AnchorTitle title="Leaders" />}>
<div className="flex w-full flex-col items-center justify-around overflow-hidden md:flex-row">
{Object.keys(leaders).map((username) => (
<div key={username}>
<LeaderData username={username} />
</div>
))}
</div>
</SecondaryCard>
{leaders && (
<LeadersListBlock leaders={leaders} icon={faArrowUpRightFromSquare} label="Leaders" />
)}

{topContributors && (
<TopContributorsList
Expand Down Expand Up @@ -241,40 +231,4 @@ const About = () => {
)
}

const LeaderData = ({ username }: { username: string }) => {
const { data, loading, error } = useQuery(GET_LEADER_DATA, {
variables: { key: username },
})
const router = useRouter()

if (loading) return <p>Loading {username}...</p>
if (error) return <p>Error loading {username}'s data</p>

const user = data?.user

if (!user) {
return <p>No data available for {username}</p>
}

const handleButtonClick = (user: User) => {
router.push(`/members/${user.login}`)
}

return (
<UserCard
avatar={user.avatarUrl}
button={{
icon: <FontAwesomeIconWrapper icon="fa-solid fa-right-to-bracket" />,
label: 'View Profile',
onclick: () => handleButtonClick(user),
}}
className="h-64 w-40 bg-inherit"
company={user.company}
description={leaders[user.login]}
location={user.location}
name={user.name || username}
/>
)
}

export default About
1 change: 1 addition & 0 deletions frontend/src/app/projects/[projectKey]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ const ProjectDetailsPage = () => {
stats={projectStats}
summary={project.summary}
title={project.name}
leaders={project.leadersTemp}
topContributors={topContributors}
topics={project.topics}
type="project"
Expand Down
12 changes: 12 additions & 0 deletions frontend/src/components/CardDetailsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'
import Link from 'next/link'
import type { DetailsCardProps } from 'types/card'
import { LeadersListBlockProps } from 'types/leaders'
import { capitalize } from 'utils/capitalize'
import { IS_PROJECT_HEALTH_ENABLED } from 'utils/credentials'
import { getSocialIcon } from 'utils/urlIconMappings'
Expand All @@ -19,6 +20,7 @@ import ChapterMapWrapper from 'components/ChapterMapWrapper'
import HealthMetrics from 'components/HealthMetrics'
import InfoBlock from 'components/InfoBlock'
import LeadersList from 'components/LeadersList'
import LeadersListBlock from 'components/LeadersListBlock'
import Milestones from 'components/Milestones'
import RecentIssues from 'components/RecentIssues'
import RecentPullRequests from 'components/RecentPullRequests'
Expand Down Expand Up @@ -49,6 +51,7 @@ const DetailsCard = ({
stats,
summary,
title,
leaders,
topContributors,
topics,
type,
Expand Down Expand Up @@ -197,6 +200,15 @@ const DetailsCard = ({
)}
</div>
)}
{leaders && (
<LeadersListBlock
label="Leaders"
leaders={leaders.reduce((acc, user) => {
acc[user.login] = ''
return acc
}, {} as LeadersListBlockProps)}
/>
)}
{topContributors && (
<TopContributorsList
icon={faUsers}
Expand Down
71 changes: 71 additions & 0 deletions frontend/src/components/LeadersListBlock.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
'use client'
import { useQuery } from '@apollo/client'
import { IconProp } from '@fortawesome/fontawesome-svg-core'
import { User } from '@sentry/nextjs'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect import for User type.

The User type should not be imported from @sentry/nextjs. This is likely meant to be a local User type definition.

-import { User } from '@sentry/nextjs'
+import { User } from 'types/user'

If the User type doesn't exist in types/user, you may need to create it or import from the correct location.

📝 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.

Suggested change
import { User } from '@sentry/nextjs'
-import { User } from '@sentry/nextjs'
+import { User } from 'types/user'
🤖 Prompt for AI Agents 
In frontend/src/components/LeadersListBlock.tsx at line 4, the User type is
incorrectly imported from '@sentry/nextjs'. Remove this import and instead
import the User type from the correct local path, such as 'types/user'. If the
User type does not exist there, create the appropriate type definition in that
location before importing it.

import { useRouter } from 'next/navigation'
import FontAwesomeIconWrapper from 'wrappers/FontAwesomeIconWrapper'
import { GET_LEADER_DATA } from 'server/queries/userQueries'
import { LeadersListBlockProps } from 'types/leaders'
import AnchorTitle from 'components/AnchorTitle'
import SecondaryCard from 'components/SecondaryCard'
import UserCard from 'components/UserCard'

const LeadersListBlock = ({
leaders,
label = 'Leaders',
icon,
}: {
leaders: LeadersListBlockProps
label?: string
icon?: IconProp
}) => {
const LeaderData = ({ username }: { username: string }) => {
const { data, loading, error } = useQuery(GET_LEADER_DATA, {
variables: { key: username },
})
const router = useRouter()

if (loading) return <p>Loading {username}...</p>
if (error) return <p>Error loading {username}'s data</p>

const user = data?.user

if (!user) {
return <p>No data available for {username}</p>
}

const handleButtonClick = (user: User) => {
router.push(`/members/${user.login}`)
}
Comment on lines +37 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix type annotation for handleButtonClick parameter.

The parameter type should match the actual user object structure from the GraphQL response, not the Sentry User type.

-    const handleButtonClick = (user: User) => {
+    const handleButtonClick = (user: any) => {
       router.push(`/members/${user.login}`)
     }

Or better yet, define a proper User type and use it consistently throughout the application.

📝 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.

Suggested change
const handleButtonClick = (user: User) => {
router.push(`/members/${user.login}`)
}
const handleButtonClick = (user: any) => {
router.push(`/members/${user.login}`)
}
🤖 Prompt for AI Agents 
In frontend/src/components/LeadersListBlock.tsx around lines 37 to 39, the
handleButtonClick function parameter is currently typed as Sentry's User type,
which does not match the actual user object structure from the GraphQL response.
To fix this, define a proper User type that reflects the GraphQL user data
structure and update the parameter type annotation of handleButtonClick to use
this new User type consistently throughout the application.


return (
<UserCard
avatar={user.avatarUrl}
button={{
icon: <FontAwesomeIconWrapper icon="fa-solid fa-right-to-bracket" />,
label: 'View Profile',
onclick: () => handleButtonClick(user),
}}
className="h-64 w-40 bg-inherit"
company={user.company}
description={leaders[user.login]}
location={user.location}
name={user.name || username}
/>
)
}
Comment on lines +22 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider potential performance implications and error handling improvements.

The nested LeaderData component creates individual GraphQL queries for each leader, which could lead to N+1 query performance issues. Consider these improvements:

  1. Performance: If multiple leaders are displayed, this creates multiple separate queries. Consider batching or using a single query with multiple variables.

  2. Error handling: The error states are basic. Consider providing more user-friendly error messages or retry mechanisms.

  3. Loading state: Individual loading states per leader might create a poor UX when multiple leaders are loading.

For better performance, consider fetching all leader data in a single query or using GraphQL batching:

// Alternative approach - single query for all leaders
const { data, loading, error } = useQuery(GET_MULTIPLE_LEADERS_DATA, {
  variables: { usernames: Object.keys(leaders) },
})
🤖 Prompt for AI Agents 
In frontend/src/components/LeadersListBlock.tsx around lines 22 to 56, the
LeaderData component runs a separate GraphQL query for each leader, causing
potential N+1 query performance issues and fragmented loading/error states.
Refactor to fetch all leader data in a single query by creating a new GraphQL
query that accepts multiple usernames as variables and returns their data
collectively. Replace the multiple individual queries with one useQuery call
that fetches all leaders at once, then map over the results to render each
leader's data. Additionally, improve error handling by providing more
user-friendly messages and consider adding retry logic. Consolidate loading
states to avoid multiple loading indicators and improve UX.


return (
<SecondaryCard icon={icon} title={<AnchorTitle title={label} />}>
<div className="flex w-full flex-col items-center justify-around overflow-hidden md:flex-row">
{Object.keys(leaders).map((username) => (
<div key={username}>
<LeaderData username={username} />
</div>
))}
</div>
</SecondaryCard>
)
}

export default LeadersListBlock
3 changes: 3 additions & 0 deletions frontend/src/server/queries/projectQueries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ export const GET_PROJECT_DATA = gql`
key
languages
leaders
leadersTemp {
login
}
Comment on lines +13 to +15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider fetching additional User fields for complete leader display.

Currently, only the login field is being fetched from leadersTemp. However, for a proper leader display component, you'll likely need additional fields such as:

  • avatarUrl for profile pictures
  • name for display names
  • company and location for professional information
  • bio for user descriptions
 leadersTemp {
   login
+  avatarUrl
+  name
+  company
+  location
+  bio
+  url
 }
📝 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.

Suggested change
leadersTemp {
login
}
leadersTemp {
login
avatarUrl
name
company
location
bio
url
}
🤖 Prompt for AI Agents 
In frontend/src/server/queries/projectQueries.ts around lines 13 to 15, the
GraphQL query for leadersTemp only fetches the login field, which is
insufficient for a complete leader display. Update the query to also fetch
avatarUrl, name, company, location, and bio fields from leadersTemp to provide
richer user information for the UI.

level
name
healthMetrics(limit: 30) {
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/types/card.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import type { Milestone } from 'types/milestone'
import type { RepositoryCardProps } from 'types/project'
import type { PullRequest } from 'types/pullRequest'
import type { Release } from 'types/release'
import type { User } from 'types/user'

export type CardProps = {
button: Button
Expand Down Expand Up @@ -53,6 +54,7 @@ export interface DetailsCardProps {
stats?: Stats[]
summary?: string
title?: string
leaders?: User[]
topContributors?: Contributor[]
topics?: string[]
type: string
Expand Down
4 changes: 4 additions & 0 deletions frontend/src/types/leaders.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
export type LeadersListProps = {
leaders: string
}

export type LeadersListBlockProps = {
[key: string]: string
}
Comment on lines +5 to +7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

LGTM! Type definition follows requirements.

The LeadersListBlockProps type correctly defines the string-to-string mapping format expected by the component, as mentioned in the PR objectives.

Consider adding a comment to clarify the structure:

+// Maps username to certification strings (e.g., { arkid15r: 'CCSP, CISSP, CSSLP' })
 export type LeadersListBlockProps = {
   [key: string]: string
 }
📝 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.

Suggested change
export type LeadersListBlockProps = {
[key: string]: string
}
// Maps username to certification strings (e.g., { arkid15r: 'CCSP, CISSP, CSSLP' })
export type LeadersListBlockProps = {
[key: string]: string
}
🤖 Prompt for AI Agents 
In frontend/src/types/leaders.ts around lines 5 to 7, add a brief comment above
the LeadersListBlockProps type definition to clarify that it represents a
mapping of string keys to string values, which helps improve code readability
and maintainability.

2 changes: 2 additions & 0 deletions frontend/src/types/project.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type { Milestone } from 'types/milestone'
import type { Organization } from 'types/organization'
import type { PullRequest } from 'types/pullRequest'
import type { Release } from 'types/release'
import { User } from 'types/user'

export type ProjectStats = {
contributors: number
Expand All @@ -25,6 +26,7 @@ export type Project = {
key: string
languages: string[]
leaders: string[]
leadersTemp: User[]
level: string
name: string
openIssuesCount?: number
Expand Down