Conversation
feat(PM-4079): Converted column based reviewer ui to row based UI
…tweaks PM-4182 tweaks for open to work status
…al-completed-profiles-report-updates PM-4198 add pagination & member details to custmer portal results
…al-completed-profiles-report-updates PM-4198 display all skills
Engagements v3
PM-4173 Remove Account Role and address sections from account settings
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| {...itemItemColumns} | ||
| data={itemData} | ||
| index={indexData} | ||
| allRows={props.data} |
There was a problem hiding this comment.
[performance]
Passing allRows={props.data} to each TableCell could lead to performance issues if props.data is large, as it increases the amount of data each cell has access to. Consider whether each cell needs access to all rows or if this can be optimized.
| } | ||
|
|
||
| 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.
| } | ||
| } | ||
|
|
||
| export async function fetchCompletedProfiles( |
There was a problem hiding this comment.
[correctness]
The countryCode parameter is optional, but if it is undefined, the query parameter will not be set. Ensure this behavior is intentional and aligns with the API's expectations.
| 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.
| return normalizeCompletedProfilesResponse(response, page, perPage) | ||
| } | ||
|
|
||
| export async function fetchMemberSkillsData(userId: string | number | undefined): Promise<UserSkill[]> { |
There was a problem hiding this comment.
[correctness]
The userId parameter is optional, but if it is undefined, the function returns an empty array. Ensure this behavior is intentional and aligns with the application's requirements.
| 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 is a significant change that could affect the layout on smaller screens. Ensure that this change has been tested across all supported screen sizes to avoid layout issues.
| @@ -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 the loop to use Promise.all or another method to handle asynchronous operations 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 might hide potential issues with the function's complexity. Consider refactoring the code to reduce complexity and improve maintainability.
| } | ||
| } | ||
|
|
||
| fetchAllMemberSkills() |
There was a problem hiding this comment.
[💡 readability]
The fetchAllMemberSkills function is defined and immediately invoked within the useEffect. Consider defining it outside the useEffect to improve readability and reduce the complexity of the hook.
| @@ -126,13 +163,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 currentPage to 1 on country change is correct, but ensure that this behavior is intended and tested, as it might affect user experience by resetting pagination unexpectedly.
| [EngagementStatus.OPEN]: 'Open', | ||
| [EngagementStatus.PENDING_ASSIGNMENT]: 'Pending Assignment', | ||
| [EngagementStatus.ACTIVE]: 'Active', | ||
| [EngagementStatus.ON_HOLD]: 'On Hold', |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that EngagementStatus.ON_HOLD is defined in the EngagementStatus enum. If it is not defined, this will cause a runtime error.
| coverLetter: '', | ||
| email: '', | ||
| mobileNumber: undefined, | ||
| mobileNumber: '', |
There was a problem hiding this comment.
[correctness]
Changing the default value of mobileNumber from undefined to an empty string ('') may lead to unintended behavior if other parts of the codebase expect mobileNumber to be undefined when not set. Ensure that this change does not affect any logic that relies on mobileNumber being undefined.
| shouldValidate: false, | ||
| }) | ||
| setValue('mobileNumber', userData.mobileNumber, { | ||
| setValue('mobileNumber', userData.mobileNumber ?? '', { |
There was a problem hiding this comment.
[correctness]
The use of the nullish coalescing operator (??) to default userData.mobileNumber to an empty string ('') may mask potential issues if userData.mobileNumber is expected to be undefined in certain scenarios. Verify that this change aligns with the intended logic and does not introduce inconsistencies.
| mobileNumber: yup | ||
| .string() | ||
| .required(requiredMessage) | ||
| .required('Mobile Number must be defined') |
There was a problem hiding this comment.
[maintainability]
The change from .required(requiredMessage) to .required('Mobile Number must be defined') introduces a hardcoded string. Consider using a constant or variable for the error message to maintain consistency and ease of updates across the codebase.
| ) | ||
| .max(20, 'Mobile number must be 20 characters or less') | ||
| .defined(), | ||
| .required('Mobile Number must be defined'), |
There was a problem hiding this comment.
[maintainability]
The .required('Mobile Number must be defined') is redundant here since .required() is already called on line 35. Consider removing this line to avoid unnecessary duplication.
| } | ||
|
|
||
| const getApiErrorMessage = (error: any): string | undefined => { | ||
| const message = error?.response?.data?.message ?? error?.data?.message ?? error?.message |
There was a problem hiding this comment.
[maintainability]
Using any as the type for the error parameter can lead to potential runtime errors and makes it difficult to understand the structure of the error object. Consider defining a more specific type for the error parameter to improve type safety and maintainability.
| return typeof message === 'string' ? message.trim() : undefined | ||
| } | ||
|
|
||
| const isPrivateEngagementAccessDeniedError = (error: any): boolean => { |
There was a problem hiding this comment.
[maintainability]
The function isPrivateEngagementAccessDeniedError uses any for the error parameter, which can lead to potential issues with type safety. Consider defining a more specific type for the error parameter to improve code robustness and maintainability.
| const status = error?.response?.status | ||
| const message = getApiErrorMessage(error) | ||
|
|
||
| if (status !== 401 && status !== 403) { |
There was a problem hiding this comment.
[correctness]
The condition status !== 401 && status !== 403 is used to check for access denial errors. Ensure that these status codes are the only ones relevant for your access control logic, as other status codes might also indicate access issues depending on the API design.
| const [, updatedTraits] = await Promise.all([ | ||
| // profile flag | ||
| props.updateMemberOpenForWork(formValue.availableForGigs), | ||
| props.updateMemberOpenForWork(!!formValue.availableForGigs), |
There was a problem hiding this comment.
[💡 correctness]
The use of !!formValue.availableForGigs ensures that the value passed to updateMemberOpenForWork is a boolean. This is a good practice to avoid potential issues with type coercion. However, ensure that formValue.availableForGigs is always defined or properly initialized to avoid unexpected behavior.
| = useState<boolean>(false) | ||
|
|
||
| const openForWork = props.profile.availableForGigs | ||
| const openForWork = props.isOpenToWork |
There was a problem hiding this comment.
[❗❗ correctness]
The change from props.profile.availableForGigs to props.isOpenToWork may affect the logic if isOpenToWork is not consistently updated with availableForGigs. Ensure that isOpenToWork accurately reflects the intended state of availability for gigs.
| {openForWork ? 'open to work' : 'not open to work'} | ||
| </p> | ||
| {openForWork === null ? ( | ||
| <p className={classNames('body-main-bold', styles.unknownOopenToWork)}> |
There was a problem hiding this comment.
[💡 style]
The class name styles.unknownOopenToWork seems to have a typo with the double 'o'. Verify the class name in the stylesheet to ensure it is correct.
| onClose: () => void | ||
| onSave: () => void | ||
| profile: UserProfile | ||
| openForWork: boolean | null |
There was a problem hiding this comment.
[correctness]
The openForWork prop is defined as boolean | null. Ensure that the rest of the codebase correctly handles the null case, as it might lead to unexpected behavior if not properly managed.
| const [formValue, setFormValue] = useState<OpenToWorkData>({ | ||
| availability: undefined, | ||
| availableForGigs: !!props.profile.availableForGigs, | ||
| availableForGigs: props.openForWork, |
There was a problem hiding this comment.
[correctness]
The change from !!props.profile.availableForGigs to props.openForWork alters the initialization logic. Ensure that openForWork is always correctly set before this component is rendered to avoid potential issues with undefined or null values.
| ...prev, | ||
| availability: openToWorkItem?.availability, | ||
| availableForGigs: !!props.profile.availableForGigs, | ||
| availableForGigs: props.openForWork, |
There was a problem hiding this comment.
[correctness]
Similar to the initialization logic, ensure that props.openForWork is correctly managed and does not introduce any unintended side effects when updating formValue.
| @@ -1,4 +1,5 @@ | |||
| /* eslint-disable complexity */ | |||
| /* eslint-disable unicorn/no-null */ | |||
There was a problem hiding this comment.
[maintainability]
Disabling the unicorn/no-null rule might lead to potential issues with null values being used where undefined might be more appropriate. Consider reviewing the necessity of using null in this context.
| (item: UserTrait) => !!item?.openToWork, | ||
| ) | ||
|
|
||
| const isOpenToWork = hasOpenToWork ? props.profile.availableForGigs : null |
There was a problem hiding this comment.
[correctness]
The use of null for isOpenToWork when hasOpenToWork is false could lead to potential issues if the consuming component does not handle null values properly. Consider using undefined instead, which is more idiomatic in JavaScript/TypeScript.
| ), | ||
| [aggregatedResults], | ||
| const reviewerRows = useMemo<SubmissionReviewerRow[]>( | ||
| () => buildSubmissionReviewerRows(aggregatedRows), |
There was a problem hiding this comment.
[performance]
The buildSubmissionReviewerRows function is used here, but its implementation is not shown in the diff. Ensure that this function is efficient and correctly handles edge cases, as it is now central to the data transformation logic.
| submission, | ||
| downloadButtonConfig, | ||
| renderer: (row: SubmissionReviewerRow) => ( | ||
| row.isFirstReviewerRow |
There was a problem hiding this comment.
[💡 readability]
Using <span /> as a placeholder for non-first reviewer rows may lead to accessibility issues or unexpected UI behavior. Consider using a more descriptive placeholder or ensuring that this does not affect the layout.
| renderer: (submission: SubmissionRow, allRows: SubmissionRow[]) => ( | ||
| props.aiReviewers && ( | ||
| renderer: (row: SubmissionReviewerRow, allRows: SubmissionReviewerRow[]) => { | ||
| if (!row.isLastReviewerRow || !props.aiReviewers) { |
There was a problem hiding this comment.
[maintainability]
The logic for determining defaultOpen in the renderer function for the ai-reviews-table column could be simplified or extracted for clarity. Consider refactoring this logic to improve maintainability.
| 0, | ||
| ), | ||
| const reviewerRows = useMemo<SubmissionReviewerRow[]>( | ||
| () => buildSubmissionReviewerRows(visibleRows), |
There was a problem hiding this comment.
[correctness]
The buildSubmissionReviewerRows function is used to transform visibleRows into reviewerRows. Ensure that this transformation correctly handles all edge cases, especially if visibleRows can contain unexpected or malformed data. Consider adding validation or error handling within buildSubmissionReviewerRows if not already present.
| renderer: (submission: SubmissionRow) => { | ||
| const reviews = submission.aggregated?.reviews ?? [] | ||
| renderer: (row: SubmissionReviewerRow) => { | ||
| const reviewDetail = row.aggregated?.reviews?.[row.reviewerIndex] |
There was a problem hiding this comment.
[correctness]
The logic for extracting reviewId from reviewDetail assumes that either reviewInfo.id or reviewId will be present. If both are absent, the function returns a placeholder. Consider adding logging or error handling to capture cases where reviewId is unexpectedly missing, as this could indicate a data integrity issue.
| return <span /> | ||
| } | ||
|
|
||
| const firstIndexForSubmission = allRows.findIndex(candidate => ( |
There was a problem hiding this comment.
[correctness]
The defaultOpen logic for CollapsibleAiReviewsRow relies on the index of the first matching row. Ensure that this logic is correct and that allRows is always in the expected order. If the order can vary, consider sorting or otherwise ensuring the correct row is identified.
| ), | ||
| [aggregatedSubmissionRows], | ||
| const reviewerRows = useMemo<SubmissionReviewerRow[]>( | ||
| () => buildSubmissionReviewerRows(aggregatedRows), |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that aggregatedRows is correctly populated before calling buildSubmissionReviewerRows. If aggregatedRows can be empty or malformed, this could lead to unexpected behavior or errors.
| ) | ||
|
|
||
| const renderActionsCell = useCallback<(submission: SubmissionRow) => JSX.Element>((submission: SubmissionRow) => { | ||
| const renderActionsCell = useCallback<(submission: SubmissionReviewerRow) => JSX.Element>(( |
There was a problem hiding this comment.
[❗❗ correctness]
The renderActionsCell function now expects SubmissionReviewerRow instead of SubmissionRow. Verify that all usages and data passed to this function are updated accordingly to prevent type errors.
| const submissionIdColumn: TableColumn<SubmissionRow> = { | ||
| className: styles.submissionColumn, | ||
| const columns = useMemo<TableColumn<SubmissionReviewerRow>[]>(() => { | ||
| const submissionIdColumn: TableColumn<SubmissionReviewerRow> = { |
There was a problem hiding this comment.
[❗❗ correctness]
The columns definition now uses SubmissionReviewerRow. Ensure that all properties accessed within the renderer functions are available on this type to avoid runtime errors.
| renderer: (submission: SubmissionRow, allRows: SubmissionRow[]) => ( | ||
| props.aiReviewers && ( | ||
| renderer: (row: SubmissionReviewerRow, allRows?: SubmissionReviewerRow[]) => { | ||
| if (!row.isLastReviewerRow || !props.aiReviewers) { |
There was a problem hiding this comment.
[correctness]
The renderer function for the ai-reviews-table column assumes allRows is defined. Consider handling the case where allRows might be undefined to prevent potential errors.
|
|
||
| submissions.forEach(submission => { | ||
| const reviews = submission.aggregated?.reviews ?? [] | ||
| const reviewCount = reviews.length || 1 |
There was a problem hiding this comment.
[❗❗ correctness]
The reviewCount is set to reviews.length || 1, which means if reviews is empty, the loop will still execute once. This could lead to incorrect data being pushed to rows if reviews is empty. Consider checking if reviews is empty before entering the loop to ensure that rows only contains valid data.
| border-top: none; | ||
| } | ||
| } | ||
| tbody tr:has(td[colspan] .TableCell_blockExpandValue .TableCell_blockCell span:only-child:empty) td { |
There was a problem hiding this comment.
[❗❗ correctness]
The :has() CSS pseudo-class is not supported in all browsers, particularly older versions of Safari and Internet Explorer. Consider using a more widely supported approach or ensure that the target browsers support this feature.
| zip?: string | ||
| }> | ||
| availableForGigs?: boolean, | ||
| availableForGigs?: boolean | null, |
There was a problem hiding this comment.
[❗❗ correctness]
Changing availableForGigs to boolean | null increases the complexity of handling this field. Ensure that all parts of the codebase that interact with this field are updated to handle null values appropriately to avoid potential null reference errors.
|
|
||
| export interface OpenToWorkData { | ||
| availableForGigs: boolean | ||
| availableForGigs: boolean | null |
There was a problem hiding this comment.
[❗❗ correctness]
Changing availableForGigs to boolean | null introduces a potential issue where the value might be null, but the rest of the code assumes it is either true or false. Ensure that all parts of the code handle the null case appropriately, especially in conditional checks.
| const OpenToWorkForm: FC<OpenToWorkFormProps> = (props: OpenToWorkFormProps) => { | ||
| function toggleOpenForWork(): void { | ||
| function handleOpenForWorkChange(e: ChangeEvent<HTMLInputElement>): void { | ||
| const openForWork = e.target.value === 'true' |
There was a problem hiding this comment.
[maintainability]
The handleOpenForWorkChange function assumes the input value will always be 'true' or 'false'. Consider adding validation or error handling to manage unexpected values, which could improve robustness.
5 participants
Mainly engagements v3 updates, but also some bug fixes