Conversation
Reports to dev
PM-4075 Fix manager comment access
PM-4075 Don't allow copilots to add/edit manager comments
…tweaks PM-4182 #time 15min qa tweaks
…al-completed-profiles-report-updates PM-4198 #time 15m qa feedback
feat(PM-4288): Open to work filter and column in profile completion table
fix(PM-3707): Show payments button in review app challenge details for creator with copilot role
…tus-in-review-table PM-3907 - ai review status
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| export const reportsRoutes: ReadonlyArray<PlatformRoute> = [ | ||
| // Reports App Root | ||
| { | ||
| authRequired: true, |
There was a problem hiding this comment.
[maintainability]
Consider adding a fallback or error boundary for the lazyLoad components to handle potential loading errors gracefully.
| domain: AppSubdomain.reports, | ||
| element: <ReportsApp />, | ||
| id: toolTitle, | ||
| rolesRequired: [ |
There was a problem hiding this comment.
[❗❗ security]
Ensure that the rolesRequired array is comprehensive and includes all necessary roles for accessing the reports. Missing roles could lead to unauthorized access or restricted access for legitimate users.
| } | ||
|
|
||
| .lockedBanner { | ||
| background-color: $black-5; |
There was a problem hiding this comment.
[maintainability]
The color #C1294F is used directly instead of a variable. Consider using a variable for consistency and easier maintenance.
| } | ||
|
|
||
| .reRunIcon { | ||
| color: $black-80; |
There was a problem hiding this comment.
[correctness]
The color $black-80 is used for the .reRunIcon. Ensure this variable is defined and consistent with the design system.
| } | ||
|
|
||
| .gatingMarker { | ||
| color: $red-140; |
There was a problem hiding this comment.
[correctness]
The color $red-140 is used for the .gatingMarker. Ensure this variable is defined and consistent with the design system.
|
|
||
| .row-passed { | ||
| .resultCol { | ||
| color: $green-140; |
There was a problem hiding this comment.
[correctness]
The color $green-140 is used for the .row-passed .resultCol. Ensure this variable is defined and consistent with the design system.
| .row-failed, | ||
| .row-failed-score { | ||
| .resultCol { | ||
| color: $red-140; |
There was a problem hiding this comment.
[correctness]
The color $red-140 is used for the .row-failed .resultCol. Ensure this variable is defined and consistent with the design system.
| <Tooltip content='Re-run the workflow'> | ||
| <IconOutline.RefreshIcon | ||
| className={classNames('icon-lg', styles.reRunIcon)} | ||
| onClick={function onClick() { handleRerun(row.run!.id) }} |
There was a problem hiding this comment.
[performance]
Using function onClick() { handleRerun(row.run!.id) } inside JSX is not optimal as it creates a new function on every render. Consider extracting this function outside of the JSX to improve performance.
|
|
||
| const { isAdmin }: UseRolePermissionsResult = useRolePermissions() | ||
| const { mutate }: FullConfiguration = useSWRConfig() | ||
| const [, setRerunningRunId] = useState<string | undefined>(undefined) |
There was a problem hiding this comment.
[💡 maintainability]
The setRerunningRunId state is defined but never used within the component. Consider removing it if it's unnecessary, or ensure it's being used correctly.
| ev.stopPropagation() | ||
| } | ||
|
|
||
| function normalizeStatus( |
There was a problem hiding this comment.
[💡 readability]
The normalizeStatus function has a high cyclomatic complexity due to multiple conditional checks. Consider refactoring to simplify the logic and improve readability.
| interface AiWorkflowRunStatusProps { | ||
| run?: Pick<AiWorkflowRun, 'status'|'score'|'workflow'|'id'> | ||
| status?: 'passed' | 'pending' | 'failed-score' | ||
| status?: 'passed' | 'pending' | 'failed-score' | 'failed' |
There was a problem hiding this comment.
[correctness]
Consider adding 'failed' to the status type definition in AiWorkflowRunStatusProps to ensure type safety and consistency with the displayStatus checks.
| label?: string | ||
| score?: number | ||
| status: 'pending' | 'failed' | 'passed' | 'failed-score' | ||
| action?: ReactNode |
There was a problem hiding this comment.
[❗❗ security]
The action prop is added as a ReactNode, which allows for a wide range of elements to be passed. Ensure that any elements passed as action are properly sanitized and validated to prevent potential security issues such as XSS attacks.
| .infoTooltipLine { | ||
| line-height: 1.4; | ||
| &.indent { | ||
| text-indent: 85px; |
There was a problem hiding this comment.
[design]
The text-indent value of 85px for the .indent class seems unusually large and could lead to layout issues, especially on smaller screens. Consider reviewing this value to ensure it aligns with the intended design across different screen sizes.
| &.indent { | ||
| text-indent: 85px; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
[💡 style]
The file is missing a newline at the end. While this is a minor issue, adding a newline can improve compatibility with various tools and version control systems.
| return `${value.toFixed(0)}%` | ||
| } | ||
|
|
||
| const AiScoreFormulaTooltip: FC<AiScoreFormulaTooltipProps> = props => { |
There was a problem hiding this comment.
[💡 readability]
Consider destructuring aiReviewConfig from props for cleaner and more readable code.
| return <></> | ||
| } | ||
|
|
||
| const formulaLines = configuredWorkflows.map((workflow, i) => { |
There was a problem hiding this comment.
[correctness]
The formulaLines array could potentially contain undefined values if workflow.weightPercent is undefined. Consider adding a check to ensure workflow.weightPercent is a valid number before formatting.
| {formulaLines.slice(1) | ||
| .map(line => ( | ||
| <div | ||
| key={line} |
There was a problem hiding this comment.
[correctness]
Using line as a key can lead to potential issues if line values are not unique. Consider using a more stable and unique key, such as the index or a unique identifier from the workflow object.
| [actionChallengeRole], | ||
| () => ( | ||
| [ADMIN, COPILOT].includes(actionChallengeRole as any) | ||
| || myResources.some(resource => resource.roleName?.toLowerCase() === 'copilot') |
There was a problem hiding this comment.
[correctness]
The use of resource.roleName?.toLowerCase() === 'copilot' assumes that roleName is always a string or undefined. If roleName can be null or another type, this could lead to unexpected behavior. Consider ensuring roleName is always a string before calling toLowerCase().
| } | ||
|
|
||
| .reviewersDropown { | ||
| .header { |
There was a problem hiding this comment.
[maintainability]
The class name .reviewersDropown was changed to .header, which might lead to confusion if .header is already used elsewhere in the codebase. Ensure that this change does not conflict with existing styles or cause unintended styling issues.
| min-width: 105px; | ||
| } | ||
|
|
||
| .table { |
There was a problem hiding this comment.
[💡 maintainability]
The commented-out code block related to .table margins should be removed if it's no longer needed. Keeping commented code can lead to confusion and clutter in the stylesheet.
| const aiReviewersCount = useMemo(() => { | ||
| const reviewersCount = props.aiReviewers.length || aiReviewConfig?.workflows?.length || 0 | ||
| return reviewersCount + 1 | ||
| }, [aiReviewConfig?.workflows?.length, props.aiReviewers.length]) |
There was a problem hiding this comment.
[correctness]
The aiReviewersCount calculation adds 1 unconditionally. Ensure this logic is correct, as it might lead to an off-by-one error if the intention is to count only existing reviewers.
| <IconOutline.InformationCircleIcon className='icon-lg' /> | ||
| </span> | ||
| </Tooltip> | ||
| {formatScore(currentDecision!.totalScore)} |
There was a problem hiding this comment.
[correctness]
The use of currentDecision! assumes currentDecision is always defined when hasScore is true. Consider adding a runtime check or assertion to ensure currentDecision is not undefined to prevent potential runtime errors.
| if (parentTr && tbody) { | ||
| const createdTr = document.createElement('tr') | ||
| const createdTd = document.createElement('td') | ||
| const colCount = parentTd?.getAttribute('colSpan') ? parentTd.colSpan : parentTr.children.length || 1 |
There was a problem hiding this comment.
[correctness]
The logic for determining colCount defaults to 1 if parentTd is null. This could lead to incorrect column spans if parentTd is unexpectedly null. Consider handling this case explicitly.
|
|
||
| import { calculateProgressAndScore } from './utils' | ||
|
|
||
| jest.mock('../../../utils', () => ({ |
There was a problem hiding this comment.
[maintainability]
Mocking roundWith2DecimalPlaces directly in the test file could lead to maintenance issues if the implementation changes. Consider using a more robust mocking strategy or testing the actual utility function to ensure consistency.
| guidelines: 'Question 1', | ||
| id: 'question-1', | ||
| requiresUpload: false, | ||
| scaleMax: 0, |
There was a problem hiding this comment.
[💡 readability]
The scaleMax and scaleMin properties are set to 0 for YES_NO type questions. If these properties are not used in calculations, consider removing them to avoid confusion.
| }, | ||
| ], | ||
| sortOrder: 1, | ||
| weight: 98, |
There was a problem hiding this comment.
[correctness]
The weight of the scorecard group is set to 98, which matches the minimumPassingScore. Ensure this is intentional and correctly reflects the scoring logic.
| * Review answers come back as `Yes`/`No` from form controls and `YES`/`NO` | ||
| * from persisted API data. Normalize both formats before applying score logic. | ||
| */ | ||
| const isAffirmativeYesNoAnswer = ( |
There was a problem hiding this comment.
[correctness]
The function isAffirmativeYesNoAnswer currently treats the string '1' as an affirmative answer. If this is not intended, consider revising the logic to avoid potential misinterpretation of numeric strings.
| @@ -1,4 +1,5 @@ | |||
| import { FC, Fragment, MouseEvent, useCallback, useMemo, useState } from 'react' | |||
| /* eslint-disable complexity */ | |||
| import { FC, Fragment, MouseEvent, useCallback, useContext, useMemo, useState } from 'react' | |||
There was a problem hiding this comment.
[maintainability]
The addition of useContext increases the complexity of the component. Ensure that the context is necessary for this component and consider if it can be refactored to reduce complexity.
| } | ||
|
|
||
| function formatScore(value?: number | null): string { | ||
| if (typeof value !== 'number' || Number.isNaN(value)) { |
There was a problem hiding this comment.
[💡 correctness]
The formatScore function uses toFixed(2), which will convert the number to a string with two decimal places. Ensure that this formatting is consistent with the rest of the application and that it meets the requirements for displaying scores.
| const aiReviewers = useMemo(() => props.aiReviewers ?? [], [props.aiReviewers]) | ||
| const aiReviewersCount = useMemo(() => (aiReviewers.length ?? 0) + 1, [aiReviewers]) | ||
| const aiReviewersCount = useMemo( | ||
| () => ((aiReviewers.length || aiReviewConfig?.workflows?.length || 0) + 1), |
There was a problem hiding this comment.
[maintainability]
The logic for calculating aiReviewersCount could be simplified. The current logic uses a fallback mechanism that might not be necessary if aiReviewers and aiReviewConfig?.workflows are always defined. Consider simplifying this logic if possible.
| const aiReviewDecisionsBySubmissionId: ChallengeDetailContextModel['aiReviewDecisionsBySubmissionId'] | ||
| = challengeDetailContext.aiReviewDecisionsBySubmissionId | ||
| const currentDecision = aiReviewDecisionsBySubmissionId[submission.id] | ||
| const hasDecisionScore = currentDecision?.totalScore !== null && currentDecision?.totalScore !== undefined |
There was a problem hiding this comment.
[💡 readability]
The check for currentDecision?.totalScore being null or undefined is repeated. Consider extracting this logic into a helper function to improve readability and reduce duplication.
| onClose={props.onClose} | ||
| title={modalTitle} | ||
| size='lg' | ||
| size='body' |
There was a problem hiding this comment.
[design]
Changing the modal size from 'lg' to 'body' might affect the UI layout. Ensure that this change is intentional and that it has been tested across different screen sizes.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| rolesRequired: [ | ||
| UserRole.administrator, | ||
| UserRole.talentManager, | ||
| ], |
There was a problem hiding this comment.
🔴 Product Manager role included in admin app access but excluded from reports app, causing redirect to denied route
The admin app root rolesRequired is set to adminReportsAccessRoles which includes UserRole.productManager (src/apps/admin/src/admin-app.routes.tsx:425). Non-administrator users are redirected by AdminHomeRedirect to reportsRootRoute (/reports). However, the reports app at src/apps/reports/src/reports-app.routes.tsx:55-58 only allows [UserRole.administrator, UserRole.talentManager] — it does not include productManager. A Product Manager will be granted entry into the admin app, be unable to access any child route (all require administratorOnlyRoles), get redirected to /reports, and then be denied access there too — resulting in a broken experience.
| rolesRequired: [ | |
| UserRole.administrator, | |
| UserRole.talentManager, | |
| ], | |
| rolesRequired: [ | |
| UserRole.administrator, | |
| UserRole.productManager, | |
| UserRole.talentManager, | |
| ], |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const aiReviewDecisionsBySubmissionId: ChallengeDetailContextModel['aiReviewDecisionsBySubmissionId'] | ||
| = challengeDetailContext.aiReviewDecisionsBySubmissionId | ||
| const currentDecision = aiReviewDecisionsBySubmissionId[submission.id] | ||
| const hasDecisionScore = currentDecision?.totalScore !== null && currentDecision?.totalScore !== undefined | ||
| const normalizedStatus = normalizeDecisionStatus(currentDecision?.status ?? undefined) | ||
|
|
||
| return ( | ||
| <Fragment key={submission.id}> |
There was a problem hiding this comment.
🔴 renderHistoryRow stale closure: aiReviewDecisionsBySubmissionId missing from useCallback deps
In SubmissionHistoryModal.tsx, the renderHistoryRow callback accesses challengeDetailContext.aiReviewDecisionsBySubmissionId (line 291) to read per-submission decision data, but this value is not included in the useCallback dependency array (lines 289-296). When AI review decisions load asynchronously after the initial render, renderHistoryRow will use the stale initial empty object {}, so the Score and Status columns in the submission history modal will always show - even after decision data arrives.
| const aiReviewDecisionsBySubmissionId: ChallengeDetailContextModel['aiReviewDecisionsBySubmissionId'] | |
| = challengeDetailContext.aiReviewDecisionsBySubmissionId | |
| const currentDecision = aiReviewDecisionsBySubmissionId[submission.id] | |
| const hasDecisionScore = currentDecision?.totalScore !== null && currentDecision?.totalScore !== undefined | |
| const normalizedStatus = normalizeDecisionStatus(currentDecision?.status ?? undefined) | |
| return ( | |
| <Fragment key={submission.id}> | |
| }, [ | |
| aiReviewersCount, | |
| challengeDetailContext.aiReviewDecisionsBySubmissionId, | |
| handleCopy, | |
| props.downloadSubmission, | |
| props.getRestriction, | |
| props.getSubmissionMeta, | |
| props.isDownloading, | |
| toggledRows, | |
| ]) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const { | ||
| decisions: aiReviewDecisions, | ||
| isLoading: isLoadingAiReviewDecisions, | ||
| }: UseFetchAiReviewDecisionsResult = useFetchAiReviewDecisions(aiReviewConfig?.id) |
There was a problem hiding this comment.
[❗❗ correctness]
The useFetchAiReviewDecisions hook is called with aiReviewConfig?.id, which could be undefined if aiReviewConfig is not yet loaded. Ensure that the hook can handle an undefined value gracefully, or add a conditional check to avoid calling the hook until aiReviewConfig is available.
| }: UseFetchAiReviewDecisionsResult = useFetchAiReviewDecisions(aiReviewConfig?.id) | ||
|
|
||
| const aiReviewDecisionsBySubmissionId = useMemo( | ||
| () => aiReviewDecisions.reduce<Record<string, typeof aiReviewDecisions[number]>>((result, decision) => { |
There was a problem hiding this comment.
[❗❗ correctness]
The reduce function used in useMemo does not handle the case where aiReviewDecisions might be undefined or null. Consider adding a default value or a guard clause to ensure aiReviewDecisions is always an array before calling reduce.
| }: SWRResponse<AiReviewConfig | undefined, Error> = useSWR<AiReviewConfig | undefined, Error>( | ||
| getAiReviewConfigCacheKey(challengeId), | ||
| { | ||
| fetcher: async (): Promise<AiReviewConfig | undefined> => { |
There was a problem hiding this comment.
[correctness]
The fetcher function should handle potential errors from fetchAiReviewConfig more robustly. Consider logging or handling unexpected errors differently to avoid silent failures.
| }: SWRResponse<AiReviewDecision[], Error> = useSWR<AiReviewDecision[], Error>( | ||
| getAiReviewDecisionsCacheKey(configId), | ||
| { | ||
| fetcher: async (): Promise<AiReviewDecision[]> => { |
There was a problem hiding this comment.
[correctness]
The fetcher function for useFetchAiReviewDecisions does not handle errors. Consider adding error handling similar to useFetchAiReviewConfig to ensure robustness.
| id: string | ||
| challengeId: string | ||
| version: number | ||
| minPassingThreshold: number |
There was a problem hiding this comment.
[💡 readability]
Consider using a more descriptive type for minPassingThreshold instead of number to ensure clarity on what values are acceptable (e.g., a percentage or a score).
| challengeId: string | ||
| version: number | ||
| minPassingThreshold: number | ||
| mode: string |
There was a problem hiding this comment.
[correctness]
The mode property is defined as a string. If there are specific modes expected, consider using a union type or an enum to restrict the values and improve type safety.
| isGating: boolean | ||
| minimumPassingScore: number | ||
| runId: string | null | ||
| runStatus: string | null |
There was a problem hiding this comment.
[correctness]
The runStatus property is defined as a string. If there are specific statuses expected, consider using a union type or an enum to restrict the values and improve type safety.
|
|
||
| export interface AiReviewDecisionBreakdown { | ||
| evaluatedAt?: string | ||
| mode?: string |
There was a problem hiding this comment.
[correctness]
The mode property is optional and defined as a string. If there are specific modes expected, consider using a union type or an enum to restrict the values and improve type safety.
| registrants: BackendResource[] | ||
| reviewers: BackendResource[] | ||
| aiReviewConfig?: AiReviewConfig | ||
| aiReviewDecisionsBySubmissionId: Record<string, AiReviewDecision> |
There was a problem hiding this comment.
[💡 design]
Consider making aiReviewDecisionsBySubmissionId optional if there are scenarios where it might not be available. This would align with the optional nature of aiReviewConfig and improve the robustness of the model.
|
|
||
| import { AiReviewConfig, AiReviewDecision } from '../models' | ||
|
|
||
| const v6BaseUrl = `${EnvironmentConfig.API.V6}` |
There was a problem hiding this comment.
[correctness]
Consider validating EnvironmentConfig.API.V6 to ensure it is a valid URL. This will prevent runtime errors if the configuration is incorrect.
| `${v6BaseUrl}/ai-review/configs/${challengeId ?? ''}` | ||
| ) | ||
|
|
||
| export const fetchAiReviewConfig = async (challengeId: string): Promise<AiReviewConfig> => ( |
There was a problem hiding this comment.
[❗❗ correctness]
The function fetchAiReviewConfig does not handle errors from xhrGetAsync. Consider adding error handling to manage potential network or server issues.
| `${v6BaseUrl}/ai-review/decisions?configId=${configId ?? ''}` | ||
| ) | ||
|
|
||
| export const fetchAiReviewDecisions = async (configId: string): Promise<AiReviewDecision[]> => ( |
There was a problem hiding this comment.
[❗❗ correctness]
The function fetchAiReviewDecisions does not handle errors from xhrGetAsync. Consider adding error handling to manage potential network or server issues.
| const [isChanged, setIsChanged] = useState(false) | ||
| const respondToAppeals = searchParams.get('respondToAppeals') === 'true' | ||
| const [isManagerEdit, setIsManagerEdit] = useState(respondToAppeals) | ||
| const hasChallengeAdminRole = useMemo( |
There was a problem hiding this comment.
[performance]
The useMemo hook is used to compute hasChallengeAdminRole, hasTopcoderAdminRole, and hasChallengeManagerRole. Ensure that myChallengeResources and myChallengeRoles are stable dependencies to avoid unnecessary recalculations. If these arrays are recreated on every render, it could lead to performance issues.
| hasChallengeManagerRole, | ||
| ], | ||
| ) | ||
| const [isManagerEdit, setIsManagerEdit] = useState(respondToAppeals && canManagerEdit) |
There was a problem hiding this comment.
[correctness]
The initial state of isManagerEdit depends on respondToAppeals and canManagerEdit. Ensure that respondToAppeals is correctly set before this component mounts, as any changes to respondToAppeals after mounting will not update the initial state.
| triggerOn='hover' | ||
| disableWrap | ||
| {workflowRuns.map(run => { | ||
| const isGating = aiReviewConfig?.workflows?.find( |
There was a problem hiding this comment.
[💡 performance]
The use of find to check for isGating in aiReviewConfig?.workflows could be inefficient if the list is large. Consider using a more efficient data structure if performance becomes an issue.
| </div> | ||
| <div className={styles.scoreInfoRow}> | ||
| <span>Min Passing Score</span> | ||
| <span>{formatScore(aiReviewConfig?.minPassingThreshold)}</span> |
There was a problem hiding this comment.
[correctness]
The use of optional chaining with aiReviewConfig?.minPassingThreshold in formatScore might lead to unexpected results if minPassingThreshold is undefined. Ensure that formatScore can handle undefined values appropriately.
|
|
||
| export function getPreferredRoleLabelByValue(value: string): string { | ||
| return preferredRoleOptions | ||
| .find(each => each.value === value)?.label as string |
There was a problem hiding this comment.
[❗❗ correctness]
The use of optional chaining (?.) followed by a type assertion (as string) can lead to runtime errors if find returns undefined. Consider providing a default value or handling the undefined case explicitly to avoid potential issues.
| const bField: unknown = b[sort.fieldName] | ||
|
|
||
| // Keep nullish values at the bottom for both sort directions. | ||
| const aValue = String(aField ?? '') |
There was a problem hiding this comment.
[correctness]
Converting aField and bField to strings using String() may lead to unexpected results if the fields contain objects or functions. Consider using a more robust conversion method or handling these cases explicitly.
PM-4265 Add overall score, fix css
| display: flex; | ||
| width: 20px; | ||
| height: 20px; | ||
| background: #fff; |
There was a problem hiding this comment.
[💡 design]
Consider adding a border or border-radius to .aiIcon for consistency with .icon, unless there's a specific design reason not to. This could improve visual consistency across the UI components.
| {props.icon && ( | ||
| <span className={classNames(styles.icon, styles[props.status])}> | ||
| <span className={classNames( | ||
| !props.isAiIcon && styles.icon, |
There was a problem hiding this comment.
[correctness]
The use of conditional logic within classNames can lead to unexpected results if props.isAiIcon is undefined or null. Consider explicitly checking for true or false to ensure clarity and correctness.
| submission: Pick<BackendSubmission, 'id'|'virusScan'> | ||
| } | ||
|
|
||
| export function normalizeDecisionStatus( |
There was a problem hiding this comment.
[maintainability]
The function normalizeDecisionStatus was changed to be exported. Ensure that this change is intentional and that the function is needed outside of this module. If not, consider keeping it as a non-exported function to limit its scope and reduce potential dependencies.
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable complexity */ | |||
There was a problem hiding this comment.
[maintainability]
Disabling ESLint's complexity rule might hide potential issues with overly complex functions. Consider refactoring the code to reduce complexity instead of disabling the rule.
| ? aiReviewDecisionsBySubmissionId?.[submissionId] | ||
| : undefined | ||
|
|
||
| const hasScore |
There was a problem hiding this comment.
[💡 readability]
The hasScore variable is checking for both null and undefined. Consider using currentDecision?.totalScore != null to simplify the check for both null and undefined.
| = currentDecision?.totalScore !== null | ||
| && currentDecision?.totalScore !== undefined | ||
|
|
||
| const overallStatus = normalizeDecisionStatus(currentDecision?.status) |
There was a problem hiding this comment.
[correctness]
Ensure that normalizeDecisionStatus handles all possible statuses correctly, especially if new statuses could be introduced in the future.
|
Closing this as we will relese dev to prod when AI Reviewer is ready... |
5 participants
Uh oh!
There was an error while loading. Please reload this page.