Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| function normalizeCompletedProfilesResponse( | ||
| raw: any, |
There was a problem hiding this comment.
[maintainability]
Consider using a more specific type than any for the raw parameter to improve type safety and maintainability.
| queryParams.set('countryCode', countryCode) | ||
| } | ||
|
|
||
| const response = await xhrGetAsync<any>( |
There was a problem hiding this comment.
[correctness]
Consider adding error handling for the xhrGetAsync call to handle potential network or API errors gracefully.
| } | ||
|
|
||
| export async function fetchMemberSkillsData(userId: string | number | undefined): Promise<UserSkill[]> { | ||
| if (!userId) { |
There was a problem hiding this comment.
[💡 correctness]
The check for !userId could be more explicit by using userId == null to handle both null and undefined cases.
| width: 100%; | ||
| border-collapse: collapse; | ||
| min-width: 420px; | ||
| min-width: 1120px; |
There was a problem hiding this comment.
[design]
Increasing the min-width of the table from 420px to 1120px could affect the layout on smaller screens. Ensure that this change has been tested across different screen sizes to maintain responsiveness.
| @@ -1,65 +1,72 @@ | |||
| /* eslint-disable react/jsx-no-bind */ | |||
| import { ChangeEvent, FC, useMemo, useState } from 'react' | |||
| /* eslint-disable no-await-in-loop */ | |||
There was a problem hiding this comment.
[performance]
Disabling no-await-in-loop can lead to performance issues if the loop contains many iterations or if the awaited operations are slow. Consider refactoring to use Promise.all to fetch data concurrently.
| /* eslint-disable react/jsx-no-bind */ | ||
| import { ChangeEvent, FC, useMemo, useState } from 'react' | ||
| /* eslint-disable no-await-in-loop */ | ||
| /* eslint-disable complexity */ |
There was a problem hiding this comment.
[maintainability]
Disabling complexity suggests that the function may be too complex. Consider breaking it down into smaller, more manageable functions to improve readability and maintainability.
| const skillsMap = new Map<string | number, UserSkill[]>() | ||
|
|
||
| for (const profile of data.data) { | ||
| if (profile.userId && !memberSkills.has(profile.userId)) { |
There was a problem hiding this comment.
[performance]
The check !memberSkills.has(profile.userId) may lead to redundant API calls if the memberSkills map is not updated correctly. Ensure that memberSkills is updated after fetching to avoid unnecessary network requests.
| @@ -126,13 +156,14 @@ export const ProfileCompletionPage: FC = () => { | |||
| value={selectedCountry} | |||
| onChange={(event: ChangeEvent<HTMLInputElement>) => { | |||
| setSelectedCountry(event.target.value || 'all') | |||
There was a problem hiding this comment.
[💡 design]
Resetting the page to 1 on country change is correct, but ensure that this behavior is communicated to the user, as it might be unexpected.
| </a> | ||
| </td> | ||
| <td>{profile.locationLabel || profile.countryLabel}</td> | ||
| <td>{profile.skillCount ?? '-'}</td> |
There was a problem hiding this comment.
[❗❗ correctness]
The profile.skillCount is used but not defined in the diff. Ensure that this property is correctly populated in the profile object to avoid displaying incorrect data.
1 participant
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-4198
What's in this PR?