Conversation
Summary by CodeRabbit
WalkthroughThis pull request updates the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 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 (5)
🪧 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 (1)
frontend/src/components/ContributorAvatar.tsx (1)
43-43: Good fix, but consider further improvements for ID formatting.The change appropriately enhances the tooltip ID's uniqueness by incorporating the repository information, which should effectively prevent collisions as mentioned in the PR description. The tooltip can now distinguish between contributors with the same login but from different repositories.
However, note that the
repositoryInfovariable (defined on lines 36-39) includes spaces and potentially special characters (e.g.,in Project Name). While this works, consider using a more ID-friendly format for better HTML practices:- id={`avatar-tooltip-${login}-${uniqueKey}-${repositoryInfo}`} + id={`avatar-tooltip-${login}-${uniqueKey}${repositoryInfo ? `-${(contributor as TopContributorsTypeGraphql).projectName?.replace(/\s+/g, '-')}` : ''}`}This would format project names by replacing spaces with hyphens for cleaner IDs.
There was a problem hiding this comment.
@rishyym0927 thanks for updating this. I'm checking it locally and for all the cases that I saw repositoryInfo is actually an empty string.
So I'm thinking what if we pass in something else to this component from the Card to differentiate the project? Could be something connected to the project or to the card itself, like index for example 🤔
|
Hey Im looking for more better and viable solution for this |
|
I updated the tooltip id generation to ensure uniqueness when the same contributor appears in multiple projects. Previously, it relied on repositoryInfo, which was often empty, leading to potential duplicate IDs.
This should prevent conflicts where contributors in the same position across projects could end up with duplicate tooltip IDs. |
|
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
I suggest look into our backend internals to understand how to fix this properly on the frontend.
|
@rishyym0927 any updates on this? Are you going to complete it or we can close it? |
Yes im working on it!! |
3 participants
Resolves #1065
The previous implementation was working but in certain cases it has the possibility of having a collision. Thus added one more variable that is the repo info to make it prone to the collisions!