[PROD RELEASE] - Reduce complexity #1027
Conversation
…loyment-docs PM-462 - update urls
PM-505 Fix default values on onboarding form
PM-505 Fix QA feedback
PM-553: fix breadcrumb url
PM-579 Add copilot request UI
PM-579 Change Restricted route logic
PM-579 Make buttons in form responsive
PM-579 QR feedback on copilot request form
V5 challenge management
feat(PM-578): Apply for copilot opportunity
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| props.onApplied() | ||
| setSuccess(true) | ||
| } catch (e) { | ||
| setSuccess(true) |
There was a problem hiding this comment.
The catch block sets setSuccess(true) even when an error occurs. Consider handling errors differently to provide feedback to the user about the failure.
| <BaseModal | ||
| onClose={props.onClose} | ||
| open | ||
| size='lg' |
There was a problem hiding this comment.
The open prop is set to true without any condition. Consider making it a prop of the component to control the modal's visibility from the parent component.
feat(PM-1067): list copilot applications
| .info { | ||
| margin-bottom: 12px; | ||
| } | ||
| } |
There was a problem hiding this comment.
It is a good practice to ensure there is a newline at the end of the file to avoid potential issues with some tools and version control systems.
| @@ -0,0 +1,224 @@ | |||
| /* eslint-disable react/jsx-no-bind */ | |||
There was a problem hiding this comment.
The use of /* eslint-disable react/jsx-no-bind */ suggests that there might be inline functions being used in JSX. Consider refactoring to avoid inline functions in JSX for better performance and to adhere to best practices.
| import { | ||
| CopilotDetailsTabViews, | ||
| getCopilotDetailsTabsConfig, | ||
| getHashFromTabId, |
There was a problem hiding this comment.
The use of useMemo here is appropriate for optimizing performance by memoizing the result of the isCopilot calculation. However, ensure that the profile object does not change frequently, as unnecessary use of useMemo can add complexity without significant performance benefits.
| const { data: copilotApplications }: { data?: CopilotApplication[] } = useCopilotApplications(opportunityId) | ||
| const { data: members }: { data?: FormattedMembers[]} = useMembers( | ||
| copilotApplications ? copilotApplications?.map(item => item.userId) : [], | ||
| ) |
There was a problem hiding this comment.
The dependency array for useCallback is empty, but it seems like opportunityId is used within the function. Consider adding opportunityId to the dependency array to ensure the callback is updated when opportunityId changes.
|
|
||
| if (!opportunityId) { | ||
| navigate(copilotRoutesMap.CopilotOpportunityList) | ||
| } |
There was a problem hiding this comment.
The dependency array for useCallback includes setShowApplyOpportunityModal, which is a setter function from useState. Typically, setter functions from useState are stable and do not need to be included in dependency arrays. Consider removing it from the dependency array.
| const navigate = useNavigate() | ||
| const [showNotFound, setShowNotFound] = useState(false) | ||
| const [showApplyOpportunityModal, setShowApplyOpportunityModal] = useState(false) | ||
| const { profile, initialized }: ProfileContextData = useContext(profileContext) |
There was a problem hiding this comment.
Consider checking if initialized is used elsewhere in the component. If not, it might be unnecessary to destructure it from profileContext.
| [profile], | ||
| ) | ||
| const isAdminOrPM: boolean = useMemo( | ||
| () => !!profile?.roles?.some(role => role === UserRole.administrator || role === UserRole.projectManager), |
There was a problem hiding this comment.
The isAdminOrPM variable could be simplified by using a Set for roles comparison if performance is a concern with larger role arrays.
| ) | ||
| const { data: copilotApplications }: { data?: CopilotApplication[] } = useCopilotApplications(opportunityId) | ||
| const { data: members }: { data?: FormattedMembers[]} = useMembers( | ||
| copilotApplications ? copilotApplications?.map(item => item.userId) : [], |
There was a problem hiding this comment.
Ensure that useMembers handles cases where copilotApplications is undefined or an empty array to prevent unnecessary API calls.
|
|
||
| const handleTabChange = useCallback((tabId: string): void => { | ||
| setActiveTab(tabId) | ||
| window.location.hash = getHashFromTabId(tabId) |
There was a problem hiding this comment.
Using window.location.hash directly can lead to unexpected behavior if there are other components or hooks that rely on the hash value. Consider using a state management solution or a context to handle hash changes more predictably.
| import useSWRInfinite, { SWRInfiniteResponse } from 'swr/infinite' | ||
|
|
||
| import { EnvironmentConfig } from '~/config' | ||
| import { xhrGetAsync, xhrPostAsync } from '~/libs/core' |
There was a problem hiding this comment.
The xhrPostAsync import is added but not used in the current code. If it's not needed, consider removing it to reduce unnecessary imports.
| export const useCopilotOpportunities = (): CopilotOpportunitiesResponse => { | ||
| const getKey = (pageIndex: number, previousPageData: CopilotOpportunity[]): string | undefined => { | ||
| if (previousPageData && previousPageData.length < PAGE_SIZE) return undefined | ||
| return ` |
There was a problem hiding this comment.
The URL string is split across multiple lines using template literals, which can introduce unintended whitespace. Consider keeping the URL on a single line or using .trim() to remove any extra spaces.
| <span className={styles.infoHeading}>Duration</span> | ||
| <span className={styles.infoValue}> | ||
| {opportunity?.numWeeks} | ||
| {' '} |
There was a problem hiding this comment.
Consider replacing the any type with a more specific type for skill to improve type safety.
| <TabsNavbar | ||
| defaultActive={activeTab} | ||
| onChange={handleTabChange} | ||
| tabs={getCopilotDetailsTabsConfig(isAdminOrPM)} |
There was a problem hiding this comment.
Ensure that getCopilotDetailsTabsConfig returns a consistent structure for tabs to avoid runtime errors.
| /> | ||
| ) | ||
| } | ||
| {activeTab === CopilotDetailsTabViews.details && <OpportunityDetails opportunity={opportunity} />} |
There was a problem hiding this comment.
Verify that OpportunityDetails component correctly handles cases where opportunity might be undefined or null.
| } | ||
| {activeTab === CopilotDetailsTabViews.details && <OpportunityDetails opportunity={opportunity} />} | ||
| {activeTab === CopilotDetailsTabViews.applications && isAdminOrPM && ( | ||
| <CopilotApplications |
There was a problem hiding this comment.
Check if CopilotApplications component requires additional props for rendering or handling edge cases.
| * @returns {CopilotOpportunityResponse} - The response containing the copilot request data. | ||
| */ | ||
| export const useCopilotOpportunity = (opportunityId?: string): CopilotOpportunityResponse => { | ||
| const url = opportunityId ? buildUrl(`${copilotBaseUrl}/copilot/opportunity/${opportunityId}`) : undefined |
There was a problem hiding this comment.
The variable copilotBaseUrl is used here, but it is not clear from the diff if it is defined or imported correctly. Please ensure that copilotBaseUrl is properly defined and imported in the file to avoid runtime errors.
| }, | ||
| ]) | ||
|
|
||
| export const CopilotDetailsTabsConfig: TabsNavItem[] = [ |
There was a problem hiding this comment.
The CopilotDetailsTabsConfig constant appears to be redundant since it duplicates the logic in getCopilotDetailsTabsConfig for the case when isAdminOrPM is true. Consider removing CopilotDetailsTabsConfig if it's not used elsewhere or if it can be replaced by calling getCopilotDetailsTabsConfig with true.
| case CopilotDetailsTabViews.applications: | ||
| return '#applications' | ||
| default: | ||
| return '#details' |
There was a problem hiding this comment.
The default case in getHashFromTabId returns '#details'. Ensure this is the intended behavior, as it might be better to handle unexpected tabId values differently, such as logging an error or throwing an exception.
| case '#applications': | ||
| return CopilotDetailsTabViews.applications | ||
| default: | ||
| return CopilotDetailsTabViews.details |
There was a problem hiding this comment.
The default case in getTabIdFromHash returns CopilotDetailsTabViews.details. Ensure this is the intended behavior, as it might be better to handle unexpected hash values differently, such as logging an error or throwing an exception.
| * @param request | ||
| * @returns | ||
| */ | ||
| export const applyCopilotOpportunity = async (opportunityId: number, request: { |
There was a problem hiding this comment.
Consider adding type validation for the request parameter to ensure it matches the expected structure. This can help prevent runtime errors if the object does not conform to the expected shape.
| ? buildUrl(`${copilotBaseUrl}/copilots/opportunity/${opportunityId}/applications`) | ||
| : undefined | ||
|
|
||
| const fetcher = (urlp: string): Promise<CopilotApplication[]> => xhrGetAsync<CopilotApplication[]>(urlp) |
There was a problem hiding this comment.
The fetcher function could be simplified by directly returning the result of xhrGetAsync. The .then(data => data) is redundant and can be removed.
| ...item, | ||
| activeProjects: member?.activeProjects || 0, | ||
| fulfilment: member?.copilotFulfillment || 0, | ||
| handle: member?.handle, |
There was a problem hiding this comment.
The handle property is being overwritten with member?.handle. Ensure that member is always defined when copilotApplications are present, or handle the case when member is undefined to avoid potential issues.
| }) | ||
| .sort((a, b) => (b.fulfilment || 0) - (a.fulfilment || 0)) : []) | ||
|
|
||
| const tableData = useMemo(getData, [props.copilotApplications, props.members]) |
There was a problem hiding this comment.
Consider adding a dependency on getData itself in the useMemo hook to ensure that the memoized value is updated correctly if getData changes.
| <Table | ||
| columns={tableColumns} | ||
| data={tableData} | ||
| disableSorting |
There was a problem hiding this comment.
The disableSorting and removeDefaultSort props seem redundant. If sorting is disabled, removing the default sort might not be necessary. Verify if both are needed.
| data={tableData} | ||
| disableSorting | ||
| removeDefaultSort | ||
| preventDefault |
There was a problem hiding this comment.
The preventDefault prop in the Table component is unclear. If this is intended to prevent default browser behavior, consider renaming it to be more descriptive of its function within the context of the Table component.
| <div> | ||
| <h2 className={styles.subHeading}> Required skills </h2> | ||
| <div className={styles.skillsContainer}> | ||
| {props.opportunity?.skills.map((skill: any) => ( |
There was a problem hiding this comment.
Consider using a more specific type than any for skill. If skill is an object with known properties, define an interface or type for it to improve type safety.
| </div> | ||
| <h2 className={styles.subHeading}> Description </h2> | ||
| <p> | ||
| {props.opportunity?.overview} |
There was a problem hiding this comment.
Consider adding a fallback or default text for props.opportunity?.overview in case it is undefined or null, to avoid rendering an empty paragraph.
| </div> | ||
| <div> | ||
| <h2 className={styles.subHeading}> Complexity </h2> | ||
| <span className={styles.textCaps}>{props.opportunity?.complexity}</span> |
There was a problem hiding this comment.
Consider adding a fallback or default text for props.opportunity?.complexity in case it is undefined or null, to avoid rendering an empty span.
| <span className={styles.textCaps}>{props.opportunity?.complexity}</span> | ||
|
|
||
| <h2 className={styles.subHeading}> Requires Communication </h2> | ||
| <span className={styles.textCaps}>{props.opportunity?.requiresCommunication}</span> |
There was a problem hiding this comment.
Consider adding a fallback or default text for props.opportunity?.requiresCommunication in case it is undefined or null, to avoid rendering an empty span.
| margin-top: $sp-6; | ||
| display: flex; | ||
| flex-direction: row; | ||
| gap: 100px; |
There was a problem hiding this comment.
Consider using a variable for the gap value instead of a hardcoded 100px to maintain consistency and ease future updates.
| .skillsContainer { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| gap: 8px; |
There was a problem hiding this comment.
Consider using a variable for the gap value instead of a hardcoded 8px to maintain consistency and ease future updates.
| .skillPill { | ||
| background-color: $teal-100; | ||
| color: white; | ||
| padding: 4px 8px; |
There was a problem hiding this comment.
Consider using a variable for the padding values instead of hardcoded 4px 8px to maintain consistency and ease future updates.
| padding: 4px 8px; | ||
| border-radius: 10px; | ||
| white-space: nowrap; | ||
| font-size: 14px; |
There was a problem hiding this comment.
Consider using a variable for the font size instead of a hardcoded 14px to maintain consistency and ease future updates.
| border-radius: 10px; | ||
| white-space: nowrap; | ||
| font-size: 14px; | ||
| } |
There was a problem hiding this comment.
Add a newline at the end of the file to follow the POSIX standard and improve compatibility with various tools.
| activeProjects: number, | ||
| fulfillment: number, | ||
| } | ||
| }[] |
There was a problem hiding this comment.
The stats property is defined as an array of objects, but it seems like it should be a single object based on the usage in membersFactory. Consider changing the type definition to reflect the actual structure.
| ): Promise<Array<{ userId: string }>> => { | ||
| let qs = '' | ||
| userIds.forEach(userId => { | ||
| qs += `&userIds[]=${userId.toLowerCase()}` |
There was a problem hiding this comment.
Consider using Array.prototype.map and Array.prototype.join to construct the query string for better readability and performance.
|
|
||
| const membersFactory = (members: Member[]): FormattedMembers[] => members.map(member => ({ | ||
| ...member, | ||
| activeProjects: member.stats.find(item => item.COPILOT.activeProjects)?.COPILOT.activeProjects || 0, |
There was a problem hiding this comment.
The find method is used twice with the same condition. Consider storing the result of find in a variable to avoid redundant operations and improve performance.
| export const useMembers = (userIds: number[]): MembersResponse => { | ||
| let qs = '' | ||
| userIds.forEach(userId => { | ||
| qs += `&userIds[]=${userId}` |
There was a problem hiding this comment.
Similar to the comment on line 35, consider using Array.prototype.map and Array.prototype.join to construct the query string for better readability and performance.
|
|
||
| const fetcher = (urlp: string): Promise<FormattedMembers[]> => xhrGetAsync<FormattedMembers[]>(urlp) | ||
| .then(data => membersFactory(data)) | ||
| .catch(() => []) |
There was a problem hiding this comment.
Catching all errors and returning an empty array might hide issues during data fetching. Consider logging the error or handling specific error cases to provide more context.
fix(PM-578): QA feedbacks on apply copilot opportunity functionality
|
|
||
| props.onApplied() | ||
| setSuccess(true) | ||
| } catch (e: any) { |
There was a problem hiding this comment.
The variable e is typed as any. Consider using a more specific error type if possible to improve type safety.
| name='Notes' | ||
| onChange={onChange} | ||
| value={notes} | ||
| error={error} |
There was a problem hiding this comment.
The error prop is being passed to the component, but it's not clear from this diff if error is defined and correctly initialized elsewhere in the code. Ensure that error is properly defined and handled to prevent potential runtime errors.
| onChange={onChange} | ||
| value={notes} | ||
| error={error} | ||
| dirty={!!error} |
There was a problem hiding this comment.
The dirty prop is being set based on the error variable. Consider whether this logic accurately reflects the intended behavior. Typically, dirty might be used to indicate if a form field has been modified, not necessarily if there's an error. Verify that this aligns with the desired functionality.
| } else { | ||
| setActiveTab('0') | ||
| } | ||
| }, [activeTabHash, isAdminOrPM]) |
There was a problem hiding this comment.
The dependency array for useEffect has been updated to include isAdminOrPM. Ensure that isAdminOrPM is a stable value or memoized to prevent unnecessary re-renders.
|
|
||
| const onApplied: () => void = useCallback(() => { | ||
| mutate(`${copilotBaseUrl}/copilots/opportunity/${opportunityId}/applications`) | ||
| mutate(`${copilotBaseUrl}/copilots/opportunity/${opportunityId}`) |
There was a problem hiding this comment.
Consider adding error handling for the mutate function to ensure that any issues during the mutation process are properly managed. This will help in maintaining the robustness of the application.
| isCopilot | ||
| && copilotApplications | ||
| && copilotApplications.length === 0 | ||
| && opportunity?.status === 'active' ? applyCopilotOpportunityButton : undefined |
There was a problem hiding this comment.
Consider using optional chaining (?.) for opportunity?.status to avoid potential runtime errors if opportunity is undefined.
| && copilotApplications.length === 0 | ||
| && opportunity?.status === 'active' ? applyCopilotOpportunityButton : undefined | ||
| } | ||
| infoComponent={(isCopilot && !(copilotApplications |
There was a problem hiding this comment.
The condition for infoComponent is complex and could benefit from being extracted into a well-named variable or function to improve readability.
| <span | ||
| className={styles.appliedText} | ||
| > | ||
| {`Applied on ${textFormatDateLocaleShortString(new Date(application.createdAt))}`} |
There was a problem hiding this comment.
Ensure that textFormatDateLocaleShortString handles invalid dates gracefully, as application.createdAt might not always be a valid date string.
|
|
||
| .textCaps { | ||
| text-transform: capitalize; | ||
| } |
There was a problem hiding this comment.
It is recommended to ensure that files end with a newline character to avoid potential issues with some tools and systems that expect a newline at the end of files.
| * @param request | ||
| * @returns | ||
| */ | ||
| export const applyCopilotOpportunity = async (opportunityId: number, request?: { |
There was a problem hiding this comment.
The request parameter is now optional, but the function implementation does not handle the case where request might be undefined. Consider adding a check to handle this scenario to avoid potential runtime errors.
| @@ -54,7 +54,6 @@ | |||
|
|
|||
| .react-responsive-modal-closeButton { | |||
| top: $sp-5; | |||
| padding: $sp-1; | |||
| border-radius: 50%; | |||
There was a problem hiding this comment.
Removing the padding from the .react-responsive-modal-closeButton might affect the button's click area and visual appearance. Consider verifying the impact on usability and design consistency across different screen sizes and devices.
Includes work on: