Skip to content

PM- 4482 Fix MM skill tooltips #1555

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

Merged
himaniraghav3 merged 6 commits into from
Mar 30, 2026
Merged

PM- 4482 Fix MM skill tooltips #1555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0571ec5
PM-4507 Don't show edit button for copilots
himaniraghav3 Mar 26, 2026
3052aa7
Remove unused code
himaniraghav3 Mar 26, 2026
212322a
PM-4482 Fix MM skills tooltip
himaniraghav3 Mar 26, 2026
6ba720a
Change for adding name to backend response
himaniraghav3 Mar 26, 2026
1fcf427
PM-4482 Add missed staged file
himaniraghav3 Mar 29, 2026
4141039
PM-4482 Change UI with backend fixes
himaniraghav3 Mar 29, 2026
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
11 changes: 1 addition & 10 deletions src/apps/review/src/pages/reviews/components/ReviewViewer/ReviewViewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,13 +180,6 @@ const ReviewViewer: FC = () => {
})
}, [challengeInfo?.id, mutate, navigate])

const hasChallengeCopilotRole = useMemo(
() => myChallengeResources.some(
resource => resource.roleName?.toLowerCase() === COPILOT.toLowerCase(),
),
[myChallengeResources],
)

const canEditScorecard = useMemo(() => {
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 26, 2026

Choose a reason for hiding this comment

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

[❗❗ correctness]
The removal of hasChallengeCopilotRole from the dependencies of canEditScorecard could lead to incorrect behavior if the copilot role is intended to influence the ability to edit the scorecard. Ensure that the copilot role is not required for this logic, or consider adding it back if it was removed unintentionally.

const challengeStatus = (challengeInfo?.status ?? '')
.toString()
Expand All @@ -203,13 +196,11 @@ const ReviewViewer: FC = () => {
reviewInfo?.committed
&& (hasChallengeAdminRole
|| hasTopcoderAdminRole
|| hasChallengeManagerRole
|| hasChallengeCopilotRole),
|| hasChallengeManagerRole),
)
}, [
challengeInfo?.status,
hasChallengeAdminRole,
hasChallengeCopilotRole,
hasChallengeManagerRole,
hasTopcoderAdminRole,
reviewInfo?.committed,
Expand Down
1 change: 1 addition & 0 deletions src/libs/core/lib/profile/user-skill.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export type UserSkillWithActivity = {
[key: string]: UserSkillActivity,
}
engagement?: UserSkillActivity
'marathon match'?: UserSkillActivity
}
} & UserSkill

Expand Down
20 changes: 20 additions & 0 deletions src/libs/shared/lib/components/skill-pill/SkillPill.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,26 @@ const SkillPill: FC<SkillPillProps> = props => {
))}
</li>
)}
{skillDetails.activity['marathon match'] && (
<li>
<div className={styles.tooltipRow}>
Marathon Matches (
{skillDetails.activity['marathon match'].lastSources?.length ?? 0}
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot Mar 29, 2026

Choose a reason for hiding this comment

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

🟡 Marathon match count displays lastSources.length instead of .count, inconsistent with all other activity types

The marathon match section uses skillDetails.activity['marathon match'].lastSources?.length ?? 0 to display the count, while every other activity type (challenges, courses, certifications, engagements) uses the .count property from UserSkillActivity. The count field represents the total number of activities, whereas lastSources is a subset containing only the most recent entries (as implied by the name). This means the displayed count for marathon matches could be lower than the actual total when there are more activities than are returned in lastSources.

Suggested change
{skillDetails.activity['marathon match'].lastSources?.length ?? 0}
{skillDetails.activity['marathon match'].count}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

):
</div>
{skillDetails.activity['marathon match'].lastSources?.map(s => (
<a
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 26, 2026

Choose a reason for hiding this comment

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

[💡 readability]
Consider adding a descriptive name for each Marathon Match link instead of using the generic text 'Marathon Match'. This will improve the clarity for users who are navigating the links.

key={s.id}
className={classNames(styles.tooltipRow, styles.padLeft)}
href={`${EnvironmentConfig.URLS.CHALLENGES_PAGE}/${s.id}`}
target='_blank'
rel='noopener noreferrer'
>
{s.name || 'Marathon Match'}
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 26, 2026

Choose a reason for hiding this comment

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

[⚠️ correctness]
Using s.name || 'Marathon Match' assumes that s.name is always a string. If s.name can be null or undefined, this is fine, but if it can be other falsy values like an empty string, this logic might unintentionally fall back to 'Marathon Match'. Consider explicitly checking for null or undefined if those are the only cases you want to handle.

</a>
))}
</li>
)}
</ul>
</>
)
Expand Down
Loading