Added tooltip when hover && repository name #1363
Conversation
Summary by CodeRabbit
WalkthroughThis set of changes updates the backend GraphQL schema and frontend components to expose a new Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (10)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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: 0
🧹 Nitpick comments (4)
frontend/src/types/user.ts (1)
89-99: Consider adding repositoryName to ItemCardPullRequests interfaceFor consistency with the
PullRequestsTypeinterface, you might want to consider adding therepositoryNameproperty to theItemCardPullRequestsinterface as well, especially if both interfaces represent similar data used in different contexts.export interface ItemCardPullRequests { createdAt: string title: string author: { login: string avatarUrl: string key: string name: string } url: string + repositoryName?: string }frontend/src/types/project.ts (1)
87-100: Consider consistency in property optionalityI noticed that
ProjectReleaseTypealready has a non-optionalrepositoryNameproperty, while the newly added property inProjectIssuesTypeis optional. If this is intentional (perhaps releases always have a repository name, while issues might not), that's fine. Otherwise, consider making the optionality consistent across similar types.frontend/src/components/RecentIssues.tsx (1)
27-37: Simplify redundant conditional check.The repository name display is implemented well, showing the repository name with an icon and making it a clickable link. However, there's a redundant conditional check in the link href.
{item?.repositoryName && ( <div className="item-center flex"> <FontAwesomeIcon icon={faFileCode} className="ml-4 mr-2 h-4 w-4" /> <Link className="text-gray-600 hover:underline dark:text-gray-400" - href={`/repositories/${item?.repositoryName ? item.repositoryName.toLowerCase() : ''}`} + href={`/repositories/${item.repositoryName.toLowerCase()}`} > <span>{item.repositoryName}</span> </Link>{' '} </div> )}The redundant check is unnecessary since we're already inside a conditional block that confirms
item?.repositoryNameexists.backend/apps/github/graphql/nodes/issue.py (1)
25-27: Consider adding a null check to the resolver.The resolver implementation correctly retrieves the repository name. However, consider adding a null check to handle edge cases where the repository might be None.
def resolve_repository_name(self, info): """Resolve the repository name.""" - return self.repository.name + return self.repository.name if self.repository else NoneWhile this is likely an edge case, adding the null check would make the code more robust.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/apps/github/graphql/nodes/issue.py(2 hunks)backend/apps/github/graphql/nodes/pull_request.py(2 hunks)frontend/src/app/members/[memberKey]/page.tsx(3 hunks)frontend/src/app/page.tsx(1 hunks)frontend/src/components/ItemCardList.tsx(3 hunks)frontend/src/components/RecentIssues.tsx(2 hunks)frontend/src/components/RecentPullRequests.tsx(3 hunks)frontend/src/components/RecentReleases.tsx(2 hunks)frontend/src/server/queries/homeQueries.ts(2 hunks)frontend/src/server/queries/organizationQueries.ts(2 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/server/queries/userQueries.ts(1 hunks)frontend/src/types/project.ts(1 hunks)frontend/src/types/user.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
frontend/src/components/RecentReleases.tsx (1)
frontend/src/types/link.ts (1)
Link(1-6)
frontend/src/components/RecentIssues.tsx (1)
frontend/src/types/link.ts (1)
Link(1-6)
frontend/src/components/ItemCardList.tsx (1)
frontend/src/types/link.ts (1)
Link(1-6)
frontend/src/components/RecentPullRequests.tsx (1)
frontend/src/types/link.ts (1)
Link(1-6)
frontend/src/app/members/[memberKey]/page.tsx (1)
frontend/src/types/user.ts (1)
user(3-17)
backend/apps/github/graphql/nodes/pull_request.py (1)
backend/apps/github/graphql/nodes/issue.py (1)
resolve_repository_name(25-27)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (27)
frontend/src/types/user.ts (1)
84-84: LGTM! Good addition of repository name field.The addition of an optional
repositoryNameproperty to thePullRequestsTypeinterface is appropriate and well-aligned with the PR objective of adding tooltip functionality when hovering over repository names.frontend/src/types/project.ts (1)
13-13: LGTM! Well-structured property addition.The addition of an optional
repositoryNameproperty to theProjectIssuesTypeinterface is appropriate and aligns well with the rest of the changes in this PR.frontend/src/app/page.tsx (1)
280-280: LGTM! Appropriate prop removal.Removing the
showAuthorprop from theRecentPullRequestscomponent makes sense given the shift from displaying author details to repository information. This change aligns well with the addition of therepositoryNameproperty to the relevant data types.frontend/src/server/queries/userQueries.ts (1)
8-8: LGTM! Well-implemented GraphQL query enhancement.Adding the
repositoryNamefield to both therecentIssuesandrecentPullRequestsGraphQL queries is a necessary and well-executed change to support the new UI features. This ensures that the frontend will receive the repository name data it needs to display tooltips on hover.Also applies to: 14-14
frontend/src/server/queries/homeQueries.ts (2)
38-38: GraphQL query field addition looks good.The addition of
repositoryNameto therecentIssuessection aligns with the PR objective of displaying repository information on hover. This field will allow the component to access and display the repository name.
55-55: GraphQL query field addition looks good.The addition of
repositoryNameto therecentPullRequestssection is consistent with the changes made to therecentIssuessection, maintaining a uniform approach across different entity types in the query.frontend/src/server/queries/organizationQueries.ts (3)
37-40: Field ordering and addition look good.The addition of
repositoryNameand reordering of fields in therecentPullRequestssection is well-implemented. GraphQL query field order doesn't affect functionality, and the new field enhances the data available to the frontend.
47-51: Field ordering and addition look good.The addition of
repositoryNameand reordering of fields in therecentReleasessection is consistent with the changes in other sections. This ensures a consistent approach across different entity types.
69-69: GraphQL query field addition looks good.The addition of
repositoryNameto therecentIssuessection is consistent with the changes made to other sections and aligns with the PR objective of displaying repository information.frontend/src/components/RecentIssues.tsx (1)
3-3: Import addition is appropriate.The addition of the
Linkimport fromnext/linkis necessary for the new repository link functionality implemented in this component.backend/apps/github/graphql/nodes/issue.py (2)
3-4: Import style improvement looks good.Moving
grapheneto its own import line improves code readability, following Python style best practices.
12-13: Adding repository_name field looks good.The addition of the
repository_namefield to theIssueNodeclass is well-implemented, using the appropriate GraphQL type.backend/apps/github/graphql/nodes/pull_request.py (2)
12-12: LGTM: Adding repository_name field to PullRequestNodeThe addition of the
repository_namefield to thePullRequestNodeclass is appropriate and aligns with your PR objective of displaying repository names in the UI.
23-25: LGTM: Well-implemented resolver methodThe resolver implementation is correct and consistent with the same pattern used in the issue node. It properly returns the repository name from the repository attribute.
frontend/src/components/RecentReleases.tsx (2)
3-3: LGTM: Appropriate tooltip importImporting the Tooltip component from @heroui/tooltip is the correct approach for implementing the hover functionality requested in the PR.
34-54: LGTM: Well-implemented tooltip for author avatarsThe tooltip implementation is well-structured with appropriate configuration:
- Shows author name or fallback to login
- Has proper delay settings for good UX
- Includes unique ID with index for accessibility
- Correctly positions the tooltip at the bottom
- Includes an arrow for better visual indication
This implementation successfully addresses the PR objective of showing a tooltip when hovering over elements.
frontend/src/components/ItemCardList.tsx (3)
2-2: LGTM: Appropriate tooltip importImporting the Tooltip component from @heroui/tooltip is consistent with the implementation in RecentReleases.tsx.
26-26: LGTM: Type definition updated for repositoryNameAdding the repositoryName property to the renderDetails function parameter type ensures type safety and proper TypeScript validation.
43-63: LGTM: Consistent tooltip implementationThe tooltip implementation here matches the pattern used in RecentReleases.tsx, providing a consistent user experience across components.
frontend/src/server/queries/projectQueries.ts (1)
22-26: LGTM: GraphQL query updated to include repositoryNameThe query has been appropriately updated to include the repositoryName field in the recentIssues object, which aligns with the backend changes made to expose this field through the GraphQL API.
frontend/src/components/RecentPullRequests.tsx (4)
1-1: Good update to imports.Updated imports to include
faFileCodefor repository icons andLinkfor navigation, aligning with the new repository name display feature.Also applies to: 3-3
10-13: Clean interface simplification.Removing the
showAuthorprop simplifies the component interface while maintaining the necessary functionality. Good practice to eliminate unused props.
15-15: Props destructuring properly updated.Props destructuring has been correctly updated to remove the unused
showAuthorprop while maintaining theshowAvatardefault value.
29-39: Well-implemented repository name display with hover functionality.The implementation of the repository name display is clean and follows best practices:
- Proper conditional rendering based on
item?.repositoryName- Appropriate icon selection with
faFileCode- Good use of Next.js Link component for navigation
- URL path construction with proper casing using
toLowerCase()- Clean hover styling with
hover:underlineThis successfully implements the tooltip functionality when hovering over repository names as specified in the PR objectives.
frontend/src/app/members/[memberKey]/page.tsx (3)
113-129: Good addition of repositoryName to issues data structure.The implementation correctly adds the
repositoryNameproperty to the mapped issues data, along with theloginfield to the author object. This provides the necessary data for the repository name tooltip feature.
131-146: Well-implemented repositoryName addition to pull requests.The addition of the
repositoryNameproperty to pull requests and theloginfield to the author object is consistent with the changes in the issues mapping. This ensures a uniform implementation of the repository tooltip feature across different data types.
148-165: Consistent implementation for releases data structure.The changes to the
formattedReleasesfunction follow the same pattern as the issues and pull requests, maintaining consistency throughout the codebase. The repositoryName is correctly added and the properties are logically ordered.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/__tests__/e2e/data/mockHomeData.ts(3 hunks)frontend/__tests__/e2e/pages/Home.spec.ts(1 hunks)frontend/__tests__/e2e/pages/ProjectDetails.spec.ts(1 hunks)frontend/__tests__/e2e/pages/RepositoryDetails.spec.ts(1 hunks)frontend/__tests__/e2e/pages/UserDetails.spec.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run Code Scan
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run CI Denendencies Scan
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (8)
frontend/__tests__/e2e/pages/RepositoryDetails.spec.ts (1)
77-77: Test updated to verify repository name display.The test has been correctly updated to verify the visibility of repository name ("Test Repo 2") instead of comment count, which aligns with the UI changes made in this PR to display repository names with tooltips.
frontend/__tests__/e2e/pages/Home.spec.ts (1)
68-68: Test correctly updated to verify repository name.The test now properly checks for the visibility of "Dependency-Track" repository name instead of comment count, which matches the UI changes implemented in this PR.
frontend/__tests__/e2e/pages/ProjectDetails.spec.ts (1)
82-82: Test properly updated to validate repository name display.The test has been correctly modified to verify the visibility of "Test Repo" repository name instead of comment count, which aligns with the UI changes to display repository names with tooltips on hover.
frontend/__tests__/e2e/pages/UserDetails.spec.ts (2)
43-43: Test correctly updated to verify repository name.The test now checks for "Test repo 1" visibility instead of comment count, which corresponds to the UI changes made in this PR to display repository names with tooltips.
58-58: Test updated to verify repository name.The test has been properly modified to check for "Test Repo 2" visibility, which aligns with the PR's focus on displaying repository names with tooltips.
frontend/__tests__/e2e/data/mockHomeData.ts (3)
118-119: LGTM! Correct repository nameThe repository name "Dependency-Track" is consistent with the URL that points to "DependencyTrack/hyades", accurately representing the parent repository.
132-133: LGTM! Correct repository nameThe repository name "BLT" matches the URL which points to "OWASP-BLT/BLT", providing accurate repository information.
114-155: Consider updating test data structure if the UI is changingBased on the PR objectives, you're adding tooltips that appear when hovering over repository names. If the UI implementation is replacing comment counts with repository names (as suggested in the AI summary), ensure that your tests are updated accordingly to validate this new behavior.
Are there corresponding test changes to verify that the repository names are displayed correctly and that tooltips function as expected when hovering?
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please look into these when you get a chance:
| login: user?.login || '', | ||
| name: user?.name || user?.login || '', | ||
| }, | ||
| commentsCount: issue.commentsCount, |
There was a problem hiding this comment.
Do we still use the comments count?
There was a problem hiding this comment.
no no . somehow i didn't noticed that
There was a problem hiding this comment.
i will do the cleanup!!!
|
Done Suggested Changes !!!! |
|
* Added tooltip when hover && repository name * fixed backend tests * fixed unit tests * fixed e2e tests * fixed some bugs & alphabatically arrangement * Added suggestions and some query cleanup * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
2 participants
Resolves #1349
Add the PR description here.