update TopContributors to use separate links for member and project navigation #1374
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change refactors the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 3
🧹 Nitpick comments (2)
frontend/src/utils/urlFormatter.ts (1)
8-10: Consider adding validation for the login name.The
getMemberUrlfunction accepts any string as input without validation, which could lead to invalid URLs if incorrect values are passed.export const getMemberUrl = (loginName = ''): string => { + // Sanitize the login name to prevent invalid URLs + const sanitizedLogin = loginName ? loginName.trim() : ''; + return `/members/${sanitizedLogin}` - return `/members/${loginName}` }frontend/src/components/TopContributors.tsx (1)
1-101: Consider splitting component into two separate components.The component currently handles two different types of content (contributors and projects) with conditional rendering and behavior based on the
typeprop. This mixed responsibility makes the component harder to maintain and understand.Consider refactoring into separate components or using more explicit prop-based configuration for the different display modes. This would improve component clarity and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/unit/pages/ProjectDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/RepositoryDetails.test.tsx(1 hunks)frontend/src/components/TopContributors.tsx(3 hunks)frontend/src/utils/urlFormatter.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/components/TopContributors.tsx (3)
frontend/src/types/link.ts (1)
Link(1-6)frontend/src/utils/urlFormatter.ts (2)
getMemberUrl(8-10)getProjectUrl(3-6)frontend/src/utils/capitalize.ts (1)
capitalize(1-4)
🔇 Additional comments (4)
frontend/__tests__/unit/pages/RepositoryDetails.test.tsx (1)
133-134: LGTM! Good switch to testing declarative links.The test has been properly updated to verify the presence and correctness of the href attribute on the contributor link, which aligns with the component's new declarative navigation approach.
frontend/__tests__/unit/pages/ProjectDetails.test.tsx (1)
131-132: LGTM! Good switch to testing declarative links.The test has been properly updated to verify the presence and correctness of the href attribute on the contributor link, which aligns with the component's new declarative navigation approach.
frontend/src/components/TopContributors.tsx (2)
37-38: Good semantic naming for clarity.Creating a named variable for the type check improves readability compared to repeating the condition.
56-61: LGTM! Good implementation of the member link.The implementation correctly wraps the contributor name in a Next.js Link component, providing proper styling and navigation to the member page.
frontend/src/utils/urlFormatter.ts
Outdated
| export const getProjectUrl = (url: string): string => { | ||
| const projectName = _.last(_.split(url, 'www-project-')) || '' | ||
| return `/projects/${projectName}` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid project URLs.
The getProjectUrl function assumes URLs will always contain 'www-project-' and extracts the project name by splitting on this pattern. However, this approach may break if the URL format changes or if an invalid URL is passed.
Consider adding validation and more robust error handling:
export const getProjectUrl = (url: string): string => {
+ if (!url) return '/projects';
const projectName = _.last(_.split(url, 'www-project-')) || ''
+ if (!projectName) {
+ console.warn('Could not extract project name from URL:', url);
+ }
return `/projects/${projectName}`
}📝 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.
| export const getProjectUrl = (url: string): string => { | |
| const projectName = _.last(_.split(url, 'www-project-')) || '' | |
| return `/projects/${projectName}` | |
| } | |
| export const getProjectUrl = (url: string): string => { | |
| if (!url) return '/projects'; | |
| const projectName = _.last(_.split(url, 'www-project-')) || '' | |
| if (!projectName) { | |
| console.warn('Could not extract project name from URL:', url); | |
| } | |
| return `/projects/${projectName}` | |
| } |
codic-yeeshu
force-pushed
the
feat/top-contributors-link-update
branch
from
249d0b9 to
c0fa49d
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/apps/github/graphql/queries/repository_contributor.py (1)
57-59: Consider using Project.nest_key instead of manual string replacement.The
Projectmodel already has anest_keyproperty that performs the samereplace("www-project-", "")operation. Consider using that property to maintain consistency and avoid duplicating logic.You could modify the query to use the
nest_keyproperty or create a database function that implements the same logic, depending on what's more efficient for your database setup.Alternatively, consider creating a utility function in Python that's used consistently across the codebase:
def get_nest_key(project_key): """Convert a project key to a nest key by removing the www-project- prefix.""" return project_key.replace("www-project-", "") if project_key else ""Then use this function both in the model's property and in this query.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/apps/github/graphql/nodes/repository_contributor.py(1 hunks)backend/apps/github/graphql/queries/repository_contributor.py(3 hunks)frontend/__tests__/e2e/data/mockHomeData.ts(1 hunks)frontend/__tests__/unit/data/mockContributeData.ts(1 hunks)frontend/__tests__/unit/data/mockProjectData.ts(1 hunks)frontend/src/components/TopContributors.tsx(3 hunks)frontend/src/server/queries/homeQueries.ts(1 hunks)frontend/src/types/contributor.ts(1 hunks)frontend/src/utils/urlFormatter.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/tests/unit/data/mockContributeData.ts
- frontend/src/types/contributor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/TopContributors.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/github/graphql/queries/repository_contributor.py (1)
backend/apps/owasp/models/project.py (1)
Project(18-279)
🔇 Additional comments (4)
frontend/src/server/queries/homeQueries.ts (1)
32-32: LGTM! Good data structure update.Replacing
projectUrlwithprojectKeyis a better approach as it separates data from presentation concerns. This change enables the new navigation pattern where links can be generated consistently from keys rather than storing complete URLs in the backend.frontend/__tests__/unit/data/mockProjectData.ts (1)
16-16: LGTM! Mock data properly updated.The mock data has been correctly updated to match the new data model, replacing
projectUrlwithprojectKey. This ensures unit tests will work with the updated TopContributors component implementation.backend/apps/github/graphql/nodes/repository_contributor.py (1)
13-13: LGTM! Good schema field update.Replacing
project_urlwithproject_keyin the GraphQL schema is a better architectural approach. It properly separates data from presentation concerns and allows for more flexible URL generation on the frontend side. This change aligns well with your goal of improving navigation in the TopContributors component.frontend/__tests__/e2e/data/mockHomeData.ts (1)
92-92: LGTM! End-to-end test data properly updated.The mock data has been correctly updated for all three contributors, replacing
projectUrlwith appropriateprojectKeyvalues that match real project identifiers. This ensures end-to-end tests will work correctly with the updated TopContributors component implementation.Also applies to: 100-100, 108-108
| export const getMemberUrl = (login: string): string => `/members/${login}` | ||
| export const getProjectUrl = (projectKey: string): string => `/projects/${projectKey}` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and JSDoc comments to the URL formatter functions.
These utility functions need input validation and documentation for better maintainability:
+/**
+ * Constructs a URL path to a member's page
+ * @param login - The GitHub username of the member
+ * @returns The URL path to the member's page
+ */
-export const getMemberUrl = (login: string): string => `/members/${login}`
+export const getMemberUrl = (login: string): string => {
+ if (!login) return '/members';
+ return `/members/${encodeURIComponent(login)}`;
+}
+/**
+ * Constructs a URL path to a project's page
+ * @param projectKey - The key identifier for the project
+ * @returns The URL path to the project's page
+ */
-export const getProjectUrl = (projectKey: string): string => `/projects/${projectKey}`
+export const getProjectUrl = (projectKey: string): string => {
+ if (!projectKey) return '/projects';
+ return `/projects/${encodeURIComponent(projectKey)}`;
+}This implementation:
- Adds fallback for empty/null/undefined inputs
- Uses
encodeURIComponentto handle special characters in parameters - Provides JSDoc comments explaining function purpose and parameters
📝 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.
| export const getMemberUrl = (login: string): string => `/members/${login}` | |
| export const getProjectUrl = (projectKey: string): string => `/projects/${projectKey}` | |
| /** | |
| * Constructs a URL path to a member's page | |
| * @param login - The GitHub username of the member | |
| * @returns The URL path to the member's page | |
| */ | |
| export const getMemberUrl = (login: string): string => { | |
| if (!login) return '/members'; | |
| return `/members/${encodeURIComponent(login)}`; | |
| } | |
| /** | |
| * Constructs a URL path to a project's page | |
| * @param projectKey - The key identifier for the project | |
| * @returns The URL path to the project's page | |
| */ | |
| export const getProjectUrl = (projectKey: string): string => { | |
| if (!projectKey) return '/projects'; | |
| return `/projects/${encodeURIComponent(projectKey)}`; | |
| } |
| contributions_count=trc["contributions_count"], | ||
| login=trc["user__login"], | ||
| name=trc["user__name"], | ||
| project_key=trc["project_key"].replace("www-project-", ""), |
There was a problem hiding this comment.
Add null check before using the replace method.
The code currently assumes project_key will always be a string. If for any reason trc["project_key"] is None, calling .replace() will cause a runtime error.
- project_key=trc["project_key"].replace("www-project-", ""),
+ project_key=trc["project_key"].replace("www-project-", "") if trc["project_key"] else "",This adds a null check to prevent potential runtime errors when accessing the project_key field.
📝 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.
| project_key=trc["project_key"].replace("www-project-", ""), | |
| project_key=trc["project_key"].replace("www-project-", "") if trc["project_key"] else "", |
…avigation (OWASP#1374) * feat: update TopContributors to use separate links for member and project navigation * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]>
2 participants
Resolves #1355
Updated the UI behavior for member and project links to ensure proper hover effects and cursor styles.
Jest test cases have been updated to reflect the changes in the navigation logic, ensuring the correct URL patterns are being used.