New work app to replace work-manager, and v6 projects API usage in copilots app#1487
New work app to replace work-manager, and v6 projects API usage in copilots app#1487
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,5 @@ | |||
| REACT_APP_GROUPS_API_URL=https://api.topcoder.com/v6/groups | |||
| REACT_APP_TERMS_API_URL=https://api.topcoder.com/v5/terms | |||
There was a problem hiding this comment.
[❗❗ correctness]
The API version for REACT_APP_TERMS_API_URL is v5, while others are v6. Ensure this is intentional and that the correct API version is being used.
| height: 400, | ||
| menubar: false, | ||
| plugins: ['table', 'link', 'textcolor', 'contextmenu'], | ||
| plugins: ['table', 'link'], |
There was a problem hiding this comment.
[correctness]
The removal of the textcolor plugin from the TinyMCE editor configuration might impact users who rely on text color customization. If this change is intentional, ensure that there are no requirements for text color editing. Otherwise, consider re-adding the plugin to maintain existing functionality.
| height: 400, | ||
| menubar: false, | ||
| plugins: ['table', 'link', 'textcolor', 'contextmenu'], | ||
| plugins: ['table', 'link'], |
There was a problem hiding this comment.
[correctness]
The removal of the contextmenu plugin could affect the user experience by disabling right-click context menus in the editor. Verify if this change aligns with the desired functionality and user requirements.
| const viewRequestDetails = useMemo(() => ( | ||
| routeParams.requestId && find(requests, { id: +routeParams.requestId }) as CopilotRequest | ||
| routeParams.requestId | ||
| ? find(requests, request => `${request.id}` === routeParams.requestId) as CopilotRequest |
There was a problem hiding this comment.
[💡 performance]
The use of string interpolation to compare request.id with routeParams.requestId could be avoided if routeParams.requestId is already a string. Consider ensuring request.id is a string when stored or retrieved, or use a more explicit conversion method if necessary.
| import { CopilotRequest } from '../models/CopilotRequest' | ||
|
|
||
| const baseUrl = `${EnvironmentConfig.API.V5}/projects` | ||
| const baseUrl = `${EnvironmentConfig.API.V6}/projects` |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that all endpoints and related logic are compatible with the new API version V6. This change might require additional updates elsewhere in the codebase to handle any differences in API responses or behavior.
| createdAt: new Date(data.createdAt), | ||
| data: undefined, | ||
| opportunity: data.copilotOpportunity?.[0], | ||
| projectId: String(data?.projectId ?? data?.data?.projectId ?? ''), |
There was a problem hiding this comment.
[correctness]
The use of String(data?.projectId ?? data?.data?.projectId ?? '') could lead to unexpected results if projectId is 0 or false. Consider explicitly checking for undefined or null to avoid coercing falsy values.
|
|
||
| export type ProjectsResponse = SWRResponse<Project[], Project[]> | ||
|
|
||
| const sleep = (ms: number): Promise<()=> void> => new Promise(resolve => { setTimeout(resolve, ms) }) |
There was a problem hiding this comment.
[correctness]
The sleep function returns a Promise<() => void>, which is misleading because the resolved value is actually undefined. Consider changing the return type to Promise<void> for clarity.
| const sleep = (ms: number): Promise<()=> void> => new Promise(resolve => { setTimeout(resolve, ms) }) | ||
| const normalizeProject = (project: Project): Project => ({ | ||
| ...project, | ||
| id: String(project.id), |
There was a problem hiding this comment.
[correctness]
The normalizeProject function converts project.id to a string. Ensure that this conversion is necessary and that all parts of the application expect id to be a string. If id is used as a number elsewhere, this could lead to inconsistencies.
| export const PageWrapper: FC<PropsWithChildren<Props>> = props => ( | ||
| <div className={classNames(styles.container, props.className)}> | ||
| <BreadCrumb list={props.breadCrumb} /> | ||
| {props.breadCrumb.length > 0 && ( |
There was a problem hiding this comment.
[correctness]
The check props.breadCrumb.length > 0 is a good optimization to avoid rendering an empty BreadCrumb component. However, ensure that props.breadCrumb is always initialized as an array to prevent potential runtime errors.
| {props.pageTitle} | ||
| </h3> | ||
| </PageHeader> | ||
| {props.titleAction |
There was a problem hiding this comment.
[💡 readability]
The conditional rendering of props.titleAction using a ternary operator is correct, but the use of undefined as the fallback is unnecessary. Consider using a logical AND (&&) operator for cleaner code: {props.titleAction && <div className={styles.blockTitleAction}>{props.titleAction}</div>}.
|
|
||
| const WorkApp: FC = () => { | ||
| const { getChildRoutes }: RouterContextData = useContext(routerContext) | ||
| const childRoutes = useMemo(() => getChildRoutes(toolTitle), [getChildRoutes]) |
There was a problem hiding this comment.
[💡 performance]
Using useMemo here is unnecessary unless getChildRoutes is computationally expensive or has side effects. Consider removing useMemo if it's not needed for performance optimization.
| const childRoutes = useMemo(() => getChildRoutes(toolTitle), [getChildRoutes]) | ||
|
|
||
| useEffect(() => { | ||
| document.body.classList.add(WORK_APP_BODY_CLASS) |
There was a problem hiding this comment.
[maintainability]
Directly manipulating the document.body.classList can lead to unexpected side effects, especially if multiple components are modifying the class list. Consider using a state management solution or a library like classnames to manage body classes more predictably.
| export const TABLE_DATE_FORMAT = 'MMM DD YYYY, HH:mm A' | ||
|
|
||
| export const PAGINATION_PER_PAGE_OPTIONS = [ | ||
| { label: '5', value: '5' }, |
There was a problem hiding this comment.
[correctness]
The value in PAGINATION_PER_PAGE_OPTIONS is a string, but it might be more appropriate to use a number type for consistency and to avoid potential type-related issues when using these values for pagination logic.
| @@ -0,0 +1,28 @@ | |||
| import { AppSubdomain, EnvironmentConfig } from '~/config' | |||
|
|
|||
| export const rootRoute: string | |||
There was a problem hiding this comment.
[💡 maintainability]
Consider using a function to determine rootRoute instead of a ternary operator. This could improve readability and maintainability, especially if the logic becomes more complex in the future.
| const defaultDate = new Date() | ||
| defaultDate.setHours(0, 0, 0, 0) | ||
|
|
||
| const daysUntilSaturday = (SATURDAY_DAY_INDEX - defaultDate.getDay() + 7) % 7 |
There was a problem hiding this comment.
[❗❗ correctness]
The calculation for daysUntilSaturday could be simplified by using (SATURDAY_DAY_INDEX - defaultDate.getDay() + 7) % 7. This ensures that the result is always non-negative, which is crucial for correctly setting the date.
| const [remarks, setRemarks] = useState<string>('') | ||
| const [weekEndingDate, setWeekEndingDate] = useState<Date | null>(() => getDefaultWeekEndingDate()) | ||
|
|
||
| const paymentTitle = useMemo( |
There was a problem hiding this comment.
[correctness]
The useMemo hook for paymentTitle depends on weekEndingDate, props.projectName, and props.engagementName. Ensure that these dependencies are correctly updated to prevent stale values from being used.
| }, []) | ||
|
|
||
| useEffect(() => { | ||
| if (!props.open) { |
There was a problem hiding this comment.
[design]
The useEffect hook resets the state whenever props.open changes. Consider whether this is the desired behavior, as it might reset the form unexpectedly if open is toggled.
| customInput={<WeekEndingInput />} | ||
| dateFormat='MM/dd/yyyy' | ||
| disabled={isSubmitting} | ||
| filterDate={isSaturday} |
There was a problem hiding this comment.
[💡 usability]
The filterDate function isSaturday ensures that only Saturdays can be selected. This is a good constraint, but ensure that users are aware of this restriction, perhaps through UI hints or documentation.
| <ul className={styles.list}> | ||
| {paymentsResult.payments.map((payment, index) => { | ||
| const paymentStatus = getPaymentStatus(payment) | ||
| const normalizedPaymentStatus = paymentStatus |
There was a problem hiding this comment.
[maintainability]
The trim() and toLowerCase() operations on paymentStatus could be moved into getPaymentStatus if this normalization is always required. This would improve maintainability by centralizing the logic.
| import { xhrGetAsync } from '~/libs/core' | ||
|
|
||
| export interface BillingAccount { | ||
| active?: boolean |
There was a problem hiding this comment.
[correctness]
The active property is optional, but its type is not specified. Consider specifying a boolean type for clarity and to avoid potential type-related issues.
|
|
||
| export interface BillingAccount { | ||
| active?: boolean | ||
| endDate?: string |
There was a problem hiding this comment.
[correctness]
The endDate property is optional and typed as a string. Consider specifying the expected date format or using a Date object to ensure consistent handling of date values.
| endDate?: string | ||
| id: number | string | ||
| name: string | ||
| startDate?: string |
There was a problem hiding this comment.
[correctness]
The startDate property is optional and typed as a string. Consider specifying the expected date format or using a Date object to ensure consistent handling of date values.
| id: number | string | ||
| name: string | ||
| startDate?: string | ||
| status?: string |
There was a problem hiding this comment.
[maintainability]
The status property is optional and typed as a string. Consider defining an enumeration for possible status values to improve type safety and maintainability.
|
|
||
| await Promise.all( | ||
| projectIds.map(async projectId => { | ||
| if (projectNameByProjectId.has(projectId)) { |
There was a problem hiding this comment.
[correctness]
Consider checking if projectId is a valid string before using it in fetchProjectById. This could prevent unnecessary API calls with invalid IDs.
| projectNameByProjectId.set(projectId, projectName) | ||
| } | ||
| } catch { | ||
| // Project lookup failures should not break engagement loading. |
There was a problem hiding this comment.
[maintainability]
Swallowing all errors in the catch block without logging or handling them might make debugging difficult. Consider logging the error or handling specific error cases.
| } | ||
|
|
||
| const projectId = getEngagementProjectId(engagement) | ||
| const projectName = projectNameByProjectId.get(projectId) |
There was a problem hiding this comment.
[correctness]
Accessing projectNameByProjectId without checking if projectId is a valid string could lead to unexpected behavior. Ensure projectId is valid before using it as a key.
| } | ||
| } | ||
|
|
||
| if (typedError?.response?.status !== 403) { |
There was a problem hiding this comment.
[correctness]
The check for typedError?.response?.status !== 403 should be more explicit. Consider using typedError?.response?.status !== 403 to ensure the status is exactly 403, as other 4xx errors might also indicate authorization issues.
| } | ||
|
|
||
| if ( | ||
| normalizedBillingAccount.active === undefined |
There was a problem hiding this comment.
[💡 maintainability]
The condition in the if statement checks for multiple properties being undefined or falsy. Consider using a utility function to improve readability and maintainability.
|
|
||
| return extractProjectTypes(response) | ||
| } catch (error) { | ||
| if (hasAuthorizationMetadataError(error)) { |
There was a problem hiding this comment.
[correctness]
The fallback mechanism for fetching project types when an authorization error occurs is a good approach. However, ensure that the fallback URL (PROJECT_METADATA_API_URL) is correct and that it provides the expected data structure.
| size='sm' | ||
| /> | ||
| </Link> | ||
| {assignedStatus |
There was a problem hiding this comment.
[maintainability]
The conditional rendering of the assignedStatus block is duplicated for both leftActions and rightActions. Consider refactoring this logic to avoid repetition and improve maintainability.
|
|
||
| <PaymentFormModal | ||
| billingAccountId={projectResult.project?.billingAccountId} | ||
| engagementName={engagementResult.engagement?.title} |
There was a problem hiding this comment.
[💡 correctness]
Ensure that engagementResult.engagement?.title is always defined before passing it to PaymentFormModal. If there's a chance it could be undefined, consider providing a default value or handling the case where it is not available.
| onCancel={() => setPaymentMember(undefined)} | ||
| onConfirm={handlePaymentSubmit} | ||
| open={!!paymentMember} | ||
| projectName={projectResult.project?.name} |
There was a problem hiding this comment.
[💡 correctness]
Ensure that projectResult.project?.name is always defined before passing it to PaymentFormModal. If there's a chance it could be undefined, consider providing a default value or handling the case where it is not available.
| return ( | ||
| <PageWrapper | ||
| backUrl={backUrl} | ||
| breadCrumb={[]} |
There was a problem hiding this comment.
[maintainability]
The breadCrumb prop is now set to an empty array, which might lead to a loss of navigation context for users. If the breadcrumb functionality is intended to be removed, ensure that this change is communicated to the team and any dependent components or tests are updated accordingly.
| } | ||
| } | ||
|
|
||
| const currentBillingAccount = params.billingAccounts.find( |
There was a problem hiding this comment.
[correctness]
Consider using Array.prototype.find with a type guard or a more specific type assertion to ensure currentBillingAccount is of type BillingAccount. This will help prevent runtime errors if billingAccounts contains unexpected types.
| const currentBillingAccount = params.billingAccounts.find( | ||
| billingAccount => String(billingAccount.id) === params.currentBillingAccountId, | ||
| ) | ||
| const billingAccountWithDetails: ProjectBillingAccount | BillingAccount | undefined |
There was a problem hiding this comment.
[design]
The variable billingAccountWithDetails is assigned using a fallback pattern. Ensure that params.projectBillingAccount and currentBillingAccount are mutually exclusive or handle cases where both might be defined to avoid unexpected behavior.
| } | ||
|
|
||
| return { | ||
| endDate: getBillingAccountDate(billingAccountWithDetails, 'endDate'), |
There was a problem hiding this comment.
[correctness]
The function getBillingAccountDate is called with billingAccountWithDetails which can be either ProjectBillingAccount or BillingAccount. Ensure that both types have the expected properties (endDate and startDate) to avoid potential runtime errors.
| return { | ||
| endDate: getBillingAccountDate(billingAccountWithDetails, 'endDate'), | ||
| id: params.currentBillingAccountId, | ||
| name: resolvedBillingAccountName || getBillingAccountName(billingAccountWithDetails), |
There was a problem hiding this comment.
[💡 maintainability]
The resolvedBillingAccountName is computed using multiple fallbacks. Consider logging or handling cases where all fallbacks result in undefined to improve debugging and ensure data integrity.
| const savedChallenge = await createChallenge({ | ||
| name: formData.name, | ||
| projectId: createProjectId, | ||
| status: CHALLENGE_STATUS.NEW, |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of roundType from the createChallenge payload may impact the challenge creation logic if roundType is required or used elsewhere in the application. Ensure that this change is intentional and that roundType is not needed for the challenge creation process.
| } | ||
|
|
||
| downloadProfileAsync(application.handle) | ||
| .catch(() => undefined) |
There was a problem hiding this comment.
[maintainability]
Catching and ignoring all errors with .catch(() => undefined) can make debugging difficult. Consider logging the error or handling specific error cases.
| </div> | ||
| <div> | ||
| <span className={styles.label}>Experience (years)</span> | ||
| <span className={styles.value}>{application.yearsOfExperience ?? '-'}</span> |
There was a problem hiding this comment.
[correctness]
Using the nullish coalescing operator (??) is appropriate here, but ensure that application.yearsOfExperience is not intended to be 0, as 0 will be replaced by '-'. If 0 is a valid value, consider using a different check.
| @import '@libs/ui/styles/includes'; | ||
|
|
||
| .modal { | ||
| width: 1240px !important; |
There was a problem hiding this comment.
[maintainability]
Using !important can make it difficult to override styles in the future, potentially leading to maintainability issues. Consider if this is necessary or if the specificity of the selector can be increased instead.
|
|
||
| .modal { | ||
| width: 1240px !important; | ||
| max-width: calc(100vw - 32px) !important; |
There was a problem hiding this comment.
[maintainability]
The use of !important here may lead to maintainability challenges. Evaluate if increasing selector specificity could be a better approach.
| } | ||
|
|
||
| .statusCell { | ||
| text-align: center !important; |
There was a problem hiding this comment.
[design]
The fixed width of 90px for .statusCell could cause layout issues on smaller screens or if the content size changes. Consider using a more flexible layout approach.
|
|
||
| .desktopTable { | ||
| overflow-x: auto; | ||
| overflow-y: visible; |
There was a problem hiding this comment.
[design]
Using overflow-y: visible; might cause content to overflow the container if the content height exceeds the container height. Ensure this behavior is intentional and won't lead to layout issues.
| columnId: 'status', | ||
| label: 'Status', | ||
| propertyName: 'status', | ||
| renderer: (data: MemberSendgridEmail) => { |
There was a problem hiding this comment.
[💡 readability]
Consider using a more descriptive variable name than data for the parameter in the renderer function to improve readability and maintainability.
| propertyName: 'status', | ||
| renderer: (data: MemberSendgridEmail) => { | ||
| const status = data.status || '-' | ||
| const isDelivered = status.toLowerCase() === 'delivered' |
There was a problem hiding this comment.
[correctness]
The status comparison is case-insensitive, which is good for robustness. Ensure that all possible status values are handled correctly, especially if new statuses are introduced in the future.
|
|
||
| return ( | ||
| <span | ||
| className={classNames( |
There was a problem hiding this comment.
[💡 style]
The use of classNames is appropriate here, but ensure that the styles emailStatusDelivered and emailStatusFailed are defined and correctly applied to avoid any styling issues.
What was broken: After saving a draft with NDA set to Yes, switching NDA to No and saving again could revert back to Yes after reload/reset. Root cause: The challenge payload sanitizer removed empty arrays globally. When NDA was turned off and terms became empty, the terms field was omitted from PATCH payload, so existing backend terms (including NDA) were retained. What was changed: Allowed empty arrays specifically for the terms payload key during challenge payload cleanup, so PATCH can explicitly clear terms. Added a focused regression test to verify transformFormDataToChallenge keeps terms: [] in the API payload. Any added/updated tests: Updated src/apps/work/src/lib/utils/challenge-editor.utils.spec.ts with a new terms mapping test that asserts empty terms arrays are preserved.
What was broken: Clearing the last selected term in Work Manager could appear to work in the UI, but after saving draft the removed term came back. Root cause: The shared multi-select field assumed the change payload for multi mode was always an array. On clear actions, non-array values could be produced, so the field failed to normalize to an empty array and the form retained stale term values. What was changed: Updated FormSelectField multi-value normalization to safely handle non-array selected values and map them to an empty array. Any added/updated tests: Added FormSelectField unit tests covering multi-select clear normalization and standard multi-value mapping.
What was broken: - Design challenges selected as "Two rounds" could be created as single-round challenges in Work Challenge Editor. Root cause: - The create flow read round type only from form state snapshot; during fast user interactions this could resolve to stale/default "Single round", so no two-round timeline template was applied. What was changed: - Added a create-round-type resolver that prioritizes the live form control value and falls back to form data. - Updated challenge creation to validate roundType in basic info checks and use the resolved round type for timeline-template selection and post-create form reset. - Wired the form element ref into ChallengeEditorForm so the create flow can read the currently checked round-type input safely. Any added/updated tests: - Extended ChallengeEditorForm.utils unit tests with coverage for create round-type resolution, including DOM-value preference, fallback behavior, and default behavior.
| toFieldValue?: (selected: SelectValue) => unknown | ||
| } | ||
|
|
||
| export function defaultToFieldValue( |
There was a problem hiding this comment.
[💡 maintainability]
The function defaultToFieldValue is exported but not used within this file. Ensure that it is intended to be used externally, or consider removing the export if it is not needed.
| isMulti: boolean, | ||
| ): unknown { | ||
| if (isMulti) { | ||
| const selectedOptions = Array.isArray(selected) |
There was a problem hiding this comment.
[correctness]
The check Array.isArray(selected) is necessary to ensure that selected is an array when isMulti is true. However, if selected is expected to always be an array when isMulti is true, consider validating this assumption earlier in the code to avoid potential misuse.
| } as const | ||
|
|
||
| const MILESTONE_METADATA_KEYS: readonly string[] = Object.values(MILESTONE_METADATA_NAMES) | ||
| const ALLOW_EMPTY_ARRAY_PAYLOAD_KEYS = new Set<string>(['terms']) |
There was a problem hiding this comment.
[maintainability]
Consider documenting the purpose of ALLOW_EMPTY_ARRAY_PAYLOAD_KEYS and its usage context. This will help maintainability by making it clear why certain keys are allowed to have empty arrays.
| const filteredEntries = Object.entries(challenge) | ||
| .filter(([key, value]) => { | ||
| if (Array.isArray(value)) { | ||
| return value.length > 0 || ALLOW_EMPTY_ARRAY_PAYLOAD_KEYS.has(key) |
There was a problem hiding this comment.
[correctness]
The condition value.length > 0 || ALLOW_EMPTY_ARRAY_PAYLOAD_KEYS.has(key) allows empty arrays for specific keys. Ensure that this behavior is intended and that all keys in ALLOW_EMPTY_ARRAY_PAYLOAD_KEYS are correctly handled downstream.
| fallbackRoundType: ChallengeEditorFormData['roundType'], | ||
| formElement: HTMLFormElement | null, | ||
| ): ChallengeEditorFormData['roundType'] { | ||
| const formRoundTypeValue = formElement |
There was a problem hiding this comment.
[maintainability]
Using FormData to extract form values can be error-prone if the form structure changes. Consider using a more robust method to access form values, such as a controlled component approach with React state.
| const navigate = useNavigate() | ||
| const isEditMode = props.isEditMode | ||
| const onRegisterLaunchAction = props.onRegisterLaunchAction | ||
| const formElementRef = useRef<HTMLFormElement>(null) |
There was a problem hiding this comment.
[💡 design]
The useRef hook is used to store a reference to the form element, which is fine for accessing DOM elements. However, ensure that this reference is only used for non-React state updates to avoid potential issues with React's rendering cycle.
| export function resolveCreateRoundType( | ||
| params: ResolveCreateRoundTypeParams, | ||
| ): ChallengeEditorFormData['roundType'] { | ||
| const normalizedFormRoundType = normalizeOptionalString(params.formRoundTypeValue) |
There was a problem hiding this comment.
[performance]
The normalizeOptionalString function is called multiple times with the same arguments. Consider storing the result in a variable to avoid redundant calls and improve performance.
| const isFieldVisible = (field: string): boolean => ( | ||
| !visibleFields || visibleFields[field] | ||
| ) | ||
| const shouldShowVolatility = isFieldVisible('volatility') |
There was a problem hiding this comment.
[maintainability]
The shouldShowVolatility logic is correct, but consider extracting it into a separate function for better readability and maintainability. This will also make it easier to test the logic independently.
…t access to the work app to only certain roles.
|
|
||
| const sortedByDisplayedStatus = [...challenges] | ||
| .sort((challengeA, challengeB) => { | ||
| const normalizedStatusA = String(challengeA.status || '') |
There was a problem hiding this comment.
[maintainability]
The normalization of status values using String(challengeA.status || '').trim().toUpperCase() is repeated for both challengeA and challengeB. Consider extracting this logic into a helper function to improve maintainability and reduce duplication.
| : -statusCompare | ||
| } | ||
|
|
||
| const statusA = getStatusText(challengeA.status) |
There was a problem hiding this comment.
[💡 performance]
The use of getStatusText(challengeA.status).toLowerCase() and getStatusText(challengeB.status).toLowerCase() could be optimized by ensuring getStatusText returns a consistent case, thus avoiding the need to call toLowerCase() here. This would improve performance slightly by reducing unnecessary string operations.
| import { WORK_MANAGER_ALLOWED_ROLES } from '../../config/access.config' | ||
| import { ErrorMessage } from '../../lib/components' | ||
|
|
||
| const requiredRolesLabel = WORK_MANAGER_ALLOWED_ROLES.join(', ') |
There was a problem hiding this comment.
[correctness]
Consider handling the case where WORK_MANAGER_ALLOWED_ROLES might be an empty array. This could result in a misleading error message like 'Required roles: '.
| domain: AppSubdomain.work, | ||
| element: <WorkApp />, | ||
| id: toolTitle, | ||
| roleErrorRoute, |
There was a problem hiding this comment.
[correctness]
The roleErrorRoute is added to the root route configuration, but it is not clear how it is being used. Ensure that this addition is necessary and correctly implemented, as it might affect routing behavior.
| if (!profile.roles || !props.rolesRequired.some(role => profile.roles.includes(role))) { | ||
| if (props.roleErrorRoute && normalizedRoleErrorPath && !isRoleErrorPath) { |
There was a problem hiding this comment.
[maintainability]
The check props.roleErrorRoute && normalizedRoleErrorPath && !isRoleErrorPath is repeated in the if condition on line 38 and the isRoleErrorPath check on line 44. Consider refactoring to avoid redundancy and improve maintainability.
| if (path === '/') { | ||
| return path | ||
| } | ||
|
|
There was a problem hiding this comment.
[💡 performance]
The normalizePath function could be optimized by using a single return statement: return path === '/' ? path : path.replace(/\/+$, ''). This reduces the number of return points and simplifies the logic.
| <RestrictedRoute loginUrl={authUrlLogin()} rolesRequired={route.rolesRequired}> | ||
| <RestrictedRoute | ||
| loginUrl={authUrlLogin()} | ||
| roleErrorRoute={route.roleErrorRoute} |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that route.roleErrorRoute is always defined and valid. If roleErrorRoute is undefined or incorrect, it might lead to unexpected behavior or errors when a user does not have the required roles.
| import { ErrorMessage } from '../../lib/components' | ||
|
|
||
| const RoleErrorPage: FC = () => ( | ||
| <ErrorMessage message='You do not have permission to access the Topcoder Work app.' /> |
There was a problem hiding this comment.
[maintainability]
Consider externalizing the error message string to a configuration or constants file. This improves maintainability by allowing easy updates to the message without modifying the component code.
| phaseId: string | undefined, | ||
| phaseNameById: Map<string, string>, | ||
| ): Scorecard[] { | ||
| const reviewerPhaseId = normalizeText(phaseId) |
There was a problem hiding this comment.
[💡 performance]
The normalizeText function is used to normalize phaseId, but it might return an empty string if phaseId is undefined. Consider handling the case where phaseId is undefined before calling normalizeText to avoid unnecessary operations.
| const reviewerPhaseName = reviewerPhaseId | ||
| ? phaseNameById.get(reviewerPhaseId) | ||
| : undefined | ||
| const normalizedReviewerPhase = normalizePhaseToken(reviewerPhaseName) |
There was a problem hiding this comment.
[correctness]
The normalizePhaseToken function is called with reviewerPhaseName, which can be undefined. Ensure that normalizePhaseToken can handle undefined inputs gracefully to avoid potential runtime errors.
| && scorecardPhaseId | ||
| && scorecardPhaseId === reviewerPhaseId | ||
| const matchesPhaseType = normalizedReviewerPhase | ||
| && normalizedScorecardType |
There was a problem hiding this comment.
[💡 readability]
The condition normalizedReviewerPhase && normalizedScorecardType && normalizedScorecardType === normalizedReviewerPhase could be simplified by using a single comparison, as both variables are already normalized strings.
| const normalizedNextPhaseId = normalizeText(nextPhaseId) | ||
| if ( | ||
| reviewer.isMemberReview === false | ||
| || !normalizedNextPhaseId |
There was a problem hiding this comment.
[💡 maintainability]
The condition reviewer.isMemberReview === false is checked multiple times in the function. Consider extracting this condition into a variable for better readability and maintainability.
| const hasDefaultScorecard = defaultScorecardId | ||
| ? matchingScorecards.some(scorecard => normalizeText(scorecard.id) === defaultScorecardId) | ||
| : false | ||
| const fallbackScorecardId = hasDefaultScorecard |
There was a problem hiding this comment.
[correctness]
Accessing matchingScorecards[0]?.id without checking if matchingScorecards is empty could lead to unexpected behavior. Consider adding a check to ensure matchingScorecards is not empty before accessing its elements.
|
|
||
| export interface CopilotApplication { | ||
| id: number, | ||
| id?: number | string, |
There was a problem hiding this comment.
[❗❗ correctness]
Changing the type of id from number to number | string introduces potential issues with type consistency. Ensure that all parts of the application that use id can handle both types, or consider maintaining a single type for consistency.
| notes?: string, | ||
| createdAt: Date, | ||
| opportunityId: string, | ||
| opportunityId?: string, |
There was a problem hiding this comment.
[correctness]
Making opportunityId optional may lead to scenarios where this field is missing. Ensure that the application logic correctly handles cases where opportunityId is undefined.
| handle?: string, | ||
| userId: number, | ||
| userHandle?: string, | ||
| userId: number | string, |
There was a problem hiding this comment.
[❗❗ correctness]
Changing the type of userId from number to number | string could lead to type inconsistencies. Verify that all usages of userId can handle both types, or consider maintaining a single type for consistency.
| const { data: copilotApplications }: { data?: CopilotApplication[] } = useCopilotApplications(opportunityId) | ||
| const appliedCopilotApplications = useMemo( | ||
| () => copilotApplications?.filter(item => item.userId === profile?.userId), | ||
| () => copilotApplications?.filter( |
There was a problem hiding this comment.
[correctness]
The conversion of userId and profile?.userId to strings before comparison ensures type consistency, which is good. However, ensure that both userId and profile?.userId are not null or undefined before this conversion to avoid unexpected results.
|
|
||
| await assignCopilotOpportunity(opportunityId, copilotApplication.id) | ||
| const applicationId = Number(copilotApplication.id) | ||
| if (!Number.isFinite(applicationId)) { |
There was a problem hiding this comment.
[correctness]
Consider using Number.isInteger() instead of Number.isFinite() to validate applicationId as an integer, since IDs are typically expected to be integers.
| const getData = (): CopilotApplication[] => (props.copilotApplications ? props.copilotApplications.map(item => { | ||
| const member = props.members && props.members.find(each => each.userId === item.userId) | ||
| const member = props.members && props.members.find( | ||
| each => String(each.userId) === String(item.userId), |
There was a problem hiding this comment.
[correctness]
Converting userId to a string for comparison may mask potential type issues. Ensure that userId is consistently typed as a string throughout the application to avoid unexpected behavior.
| activeProjects: member?.activeProjects || 0, | ||
| fulfilment: member?.copilotFulfillment || 0, | ||
| handle: member?.handle, | ||
| handle: member?.handle || item.userHandle || item.handle, |
There was a problem hiding this comment.
[correctness]
The fallback logic for handle could lead to unexpected results if item.userHandle or item.handle are not defined or incorrect. Consider validating these fields before using them as fallbacks.
| * @returns {MembersResponse} - The response containing the list of members data | ||
| */ | ||
| export const useMembers = (userIds: number[]): MembersResponse => { | ||
| export const useMembers = (userIds: Array<number | string>): MembersResponse => { |
There was a problem hiding this comment.
[correctness]
The change to allow userIds to be an array of number | string can lead to inconsistent behavior if not handled properly. Consider converting all userId values to strings before appending them to the query string to ensure uniformity and prevent potential issues with type coercion.
1 participant
https://work.topcoder-dev.com