PM-2178 merge review UI #1309
PM-2178 merge review UI #1309
Conversation
…M-2178_merge-review-ui
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| export const challengeDetailRouteId = ':challengeId' | ||
| export const scorecardRouteId = 'scorecard' | ||
| export const aiScorecardRouteId = 'ai-scorecard' | ||
| export const aiScorecardRouteId = 'scorecard' |
There was a problem hiding this comment.
[❗❗ correctness]
The aiScorecardRouteId and scorecardRouteId are both set to 'scorecard'. This could lead to confusion or errors if both are intended to represent different routes. Consider using distinct identifiers for clarity and to avoid potential routing conflicts.
| return aiRunStatus(props.run) | ||
| } | ||
|
|
||
| return '' |
There was a problem hiding this comment.
[correctness]
Returning an empty string for status when neither props.status nor props.run is provided might lead to unexpected behavior. Consider handling this case more explicitly, perhaps by throwing an error or providing a default status.
src/apps/review/src/lib/components/Scorecard/ScorecardViewer/ScorecardGroup/ScorecardGroup.tsx
Outdated
Show resolved
Hide resolved
src/apps/review/src/lib/components/Scorecard/ScorecardViewer/ScorecardGroup/ScorecardGroup.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const AiFeedback: FC<AiFeedbackProps> = props => { | ||
| const { aiFeedbackItems }: ScorecardViewerContextValue = useScorecardContext() | ||
| const { aiFeedbackItems, scoreMap }: ScorecardViewerContextValue = useScorecardViewerContext() |
There was a problem hiding this comment.
[correctness]
The scoreMap is accessed with props.question.id cast to a string. Ensure that props.question.id is always a valid key in scoreMap to avoid potential runtime errors. Consider adding a check or handling for cases where the key might not exist in scoreMap.
| </p> | ||
| )} | ||
| {feedback.content} | ||
| <MarkdownReview value={feedback.content} /> |
There was a problem hiding this comment.
[❗❗ security]
Rendering feedback.content using MarkdownReview assumes that feedback.content is always a valid markdown string. Consider adding validation or sanitization to ensure that the content is safe and correctly formatted before rendering.
There was a problem hiding this comment.
@vas3a very nice - Markdown support for AI generated feedback, right?!
| scoreMap, | ||
| }: ScorecardViewerContextValue = useScorecardViewerContext() | ||
|
|
||
| const answer = useMemo(() => ( |
There was a problem hiding this comment.
[💡 performance]
Using useMemo here might be unnecessary since string concatenation and logical OR operations are inexpensive. Consider removing useMemo unless you have a specific performance concern.
| className={styles.wrap} | ||
| score={( | ||
| <ScorecardScore | ||
| score={scoreMap.get(props.question.id as string) ?? 0} |
There was a problem hiding this comment.
[correctness]
Ensure that props.question.id is always a string or handle the case where it might not be. Using as string could lead to runtime errors if id is not a string.
| :global(.filledButton) { | ||
| font-size: 14px; | ||
| } | ||
| :global(.filledButton) { |
There was a problem hiding this comment.
[❗❗ correctness]
The :global(.filledButton) selector is defined twice with different font sizes. This could lead to confusion or unexpected behavior. Consider consolidating these rules to ensure consistent styling.
| display: flex; | ||
| align-items: center; | ||
| gap: $sp-2; | ||
| color: #0D61BF; |
There was a problem hiding this comment.
[maintainability]
The color #0D61BF is hardcoded. Consider using a CSS variable for colors to maintain consistency and make future changes easier.
|
|
||
| if (props.scorecardQuestion.type === 'SCALE') { | ||
| const length = props.scorecardQuestion.scaleMax - props.scorecardQuestion.scaleMin + 1 | ||
| return Array.from( |
There was a problem hiding this comment.
[💡 readability]
The use of Array.from(new Array(length), ...) can be simplified to Array.from({ length }, ...) for better readability.
| resolver: yupResolver(formAppealResponseSchema), | ||
| }) | ||
|
|
||
| const onSubmit = useCallback((formData: FormAppealResponse) => { |
There was a problem hiding this comment.
[correctness]
The onSubmit function does not handle the case where addAppealResponse is undefined. Consider adding a check or a fallback to prevent potential runtime errors.
| }, [addAppealResponse, props.appeal, props.reviewItem, updatedResponse]) | ||
|
|
||
| useEffect(() => { | ||
| setAppealResponse(props.appeal.appealResponse?.content || '') |
There was a problem hiding this comment.
[correctness]
The useEffect dependency array includes props.appeal.appealResponse?.content, which is a nested property. Ensure that changes to this property are correctly triggering the effect as expected.
| :global(.cancelButton) { | ||
| font-size: 14px; | ||
| } | ||
| :global(.filledButton) { |
There was a problem hiding this comment.
[❗❗ correctness]
The :global(.filledButton) selector is defined twice with different font sizes (14px and 16px). This could lead to unexpected styling behavior. Consider consolidating these definitions to ensure consistent styling.
| } | ||
|
|
||
| &:hover { | ||
| color: darken(#0D61BF, 10%); |
There was a problem hiding this comment.
[maintainability]
The use of darken(#0D61BF, 10%) relies on a Sass function. Ensure that the build process supports this function to avoid runtime errors.
| } | ||
|
|
||
| // eslint-disable-next-line complexity | ||
| const ReviewComment: FC<ReviewCommentProps> = props => { |
There was a problem hiding this comment.
[maintainability]
The ReviewComment component has a high cyclomatic complexity due to multiple conditional checks and nested components. Consider refactoring to improve readability and maintainability.
...s/Scorecard/ScorecardViewer/ScorecardQuestion/ReviewResponse/ReviewComment/ReviewComment.tsx
Show resolved
Hide resolved
| }) | ||
|
|
||
| const onSubmit = useCallback((data: FormAppealResponse) => { | ||
| if (addAppeal) { |
There was a problem hiding this comment.
[❗❗ correctness]
The onSubmit function relies on the addAppeal function being defined. Consider adding a check or a default no-op function to prevent potential runtime errors if addAppeal is undefined.
| > | ||
| }) { | ||
| return ( | ||
| <FieldMarkdownEditor |
There was a problem hiding this comment.
[performance]
The FieldMarkdownEditor component is used within a render prop function. Ensure that this component is optimized for frequent re-renders, as render prop functions can lead to performance issues if not handled carefully.
| const ReviewComments: FC<ReviewCommentsProps> = props => { | ||
| const comments = useMemo(() => ( | ||
| (props.reviewItem.reviewItemComments || []).filter(c => c.content || c.appeal || props.mappingAppeals?.[c.id]) | ||
| ).sort((a, b) => a.sortOrder - b.sortOrder), [props.reviewItem.reviewItemComments, props.mappingAppeals]) |
There was a problem hiding this comment.
[performance]
The useMemo dependency array includes props.mappingAppeals, which is an object. If mappingAppeals is a new object on each render, this will cause useMemo to recompute every time, negating its benefits. Consider using a stable reference or a specific property of mappingAppeals if possible.
...Scorecard/ScorecardViewer/ScorecardQuestion/ReviewResponse/ReviewComments/ReviewComments.tsx
Show resolved
Hide resolved
| } | ||
|
|
||
| &:hover { | ||
| color: darken(#0D61BF, 10%); |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a CSS variable for the color #0D61BF to maintain consistency and ease future updates.
| .addButton { | ||
| align-self: flex-start; | ||
| padding: 6px 12px; | ||
| background-color: #007bff; |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a CSS variable for the background color #007bff to maintain consistency and ease future updates.
| margin-top: 8px; | ||
|
|
||
| &:hover:not(:disabled) { | ||
| background-color: #0056b3; |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a CSS variable for the background color #0056b3 to maintain consistency and ease future updates.
...orecardViewer/ScorecardQuestion/ReviewResponse/ReviewManagerComment/ReviewManagerComment.tsx
Show resolved
Hide resolved
| }, [props.managerComment]) | ||
|
|
||
| if (!props.managerComment && !isManagerEdit) { | ||
| return <></> |
There was a problem hiding this comment.
[correctness]
Returning an empty fragment (<></>) when !props.managerComment && !isManagerEdit might lead to unexpected UI behavior. Consider returning null instead to explicitly indicate no rendering.
| option: SingleValue<SelectOption>, | ||
| ): void { | ||
| controlProps.field.onChange( | ||
| (option as SelectOption | null)?.value || '', |
There was a problem hiding this comment.
[💡 readability]
The use of (option as SelectOption | null)?.value || '' could be simplified to option?.value ?? '' for better readability.
|
|
||
| .questionText { | ||
| font-weight: bold; | ||
| font-weight: 600; |
There was a problem hiding this comment.
[design]
Changing font-weight from bold to 600 may affect the visual consistency if other elements use bold. Ensure this change aligns with the overall design system.
.../review/src/lib/components/Scorecard/ScorecardViewer/ScorecardQuestion/ScorecardQuestion.tsx
Show resolved
Hide resolved
.../review/src/lib/components/Scorecard/ScorecardViewer/ScorecardQuestion/ScorecardQuestion.tsx
Show resolved
Hide resolved
.../review/src/lib/components/Scorecard/ScorecardViewer/ScorecardQuestion/ScorecardQuestion.tsx
Show resolved
Hide resolved
.../review/src/lib/components/Scorecard/ScorecardViewer/ScorecardQuestion/ScorecardQuestion.tsx
Show resolved
Hide resolved
|
|
||
| .answerSection { | ||
| background: #f6f7f8; | ||
| margin: 0 -1*$sp-4 -1*$sp-4; |
There was a problem hiding this comment.
[maintainability]
The use of negative margins (margin: 0 -1*$sp-4 -1*$sp-4;) can lead to layout issues and make the component harder to maintain. Consider using padding or adjusting the layout structure to achieve the desired spacing.
|
|
||
| .hasError { | ||
| border: 1px solid #d32f2f; | ||
| margin-left: -1*$sp-4; |
There was a problem hiding this comment.
[maintainability]
Negative margins (margin-left: -1*$sp-4; margin-right: -1*$sp-4;) can cause unexpected layout shifts and make the component harder to maintain. Consider using padding or adjusting the layout structure to achieve the desired spacing.
| const isExpanded = toggledItems[props.question.id!] ?? false | ||
| const toggle = useCallback((): void => { | ||
| toggleItem(props.question.id!) | ||
| }, [toggleItem]) |
There was a problem hiding this comment.
[correctness]
The dependency array for useCallback should include props.question.id to ensure the toggle function updates correctly when the question ID changes.
| append, | ||
| }: UseFieldArrayReturn<FormReviews, 'reviews.0.comments', 'id'> | ||
| = useFieldArray({ | ||
| control: control!, |
There was a problem hiding this comment.
[correctness]
The non-null assertion control! assumes control is always defined. Consider adding a check or handling the case where control might be undefined to prevent potential runtime errors.
| onChange={function onChange( | ||
| option: SingleValue<{ label: string; value: string }>, | ||
| ) { | ||
| controlProps.field.onChange( |
There was a problem hiding this comment.
[correctness]
The type assertion (option as SelectOption) assumes that option will always be of type SelectOption. Consider adding a type guard or validation to ensure option is of the expected type before accessing its properties.
| onChange={function onChange( | ||
| option: SingleValue<{label: string; value: string}>, | ||
| ) { | ||
| controlProps.field.onChange( |
There was a problem hiding this comment.
[correctness]
Similar to line 209, the type assertion (option as SelectOption) assumes that option will always be of type SelectOption. Consider adding a type guard or validation to ensure option is of the expected type before accessing its properties.
...ts/Scorecard/ScorecardViewer/ScorecardQuestion/ScorecardQuestionRow/ScorecardQuestionRow.tsx
Show resolved
Hide resolved
| import styles from './ScorecardScore.module.scss' | ||
|
|
||
| interface ScorecardScoreProps { | ||
| score: number |
There was a problem hiding this comment.
[❗❗ correctness]
The scaleMax property has been removed from ScorecardScoreProps, but it is still used in the calculation logic in the previous version. Ensure that this change is intentional and that the removal of scaleMax does not affect the correctness of the score calculation.
| const ScorecardScore: FC<ScorecardScoreProps> = props => ( | ||
| <div className={styles.wrap}> | ||
| <strong> | ||
| {props.score.toFixed(2)} |
There was a problem hiding this comment.
[design]
Using toFixed(2) on props.score and props.weight ensures that the numbers are displayed with two decimal places. However, this converts the numbers to strings, which might not be the intended behavior if further numerical operations are needed. Ensure that this conversion aligns with the intended use case.
| import { ScorecardScore } from '../ScorecardScore' | ||
| import { ScorecardViewerContextValue, useScorecardContext } from '../ScorecardViewer.context' | ||
| import { calcSectionScore } from '../utils' | ||
| import { ScorecardViewerContextValue, useScorecardViewerContext } from '../ScorecardViewer.context' |
There was a problem hiding this comment.
[❗❗ correctness]
The import of calcSectionScore has been removed, but it was used to calculate the score previously. Ensure that the new approach using scoreMap provides equivalent functionality and correctness.
| <ScorecardScore | ||
| score={score} | ||
| scaleMax={1} | ||
| score={scoreMap.get(props.section.id as string) ?? 0} |
There was a problem hiding this comment.
[correctness]
Using scoreMap.get(props.section.id as string) ?? 0 assumes that scoreMap will always have a valid entry for each section ID. Consider handling cases where the section ID might not be present in scoreMap to avoid potential issues.
| <ScorecardScore | ||
| score={props.score ?? 0} | ||
| scaleMax={100} | ||
| weight={100} |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of scaleMax={100} from the ScorecardScore component might affect the component's behavior if it relies on this prop for scaling calculations. Ensure that the ScorecardScore component can handle the absence of this prop without any issues.
|
|
||
| export const ScorecardViewerContextProvider: FC<ScorecardViewerContextProps> = props => { | ||
| const [toggledItems, setToggledItems] = useState<{[key: string]: boolean}>({}) | ||
| const collapsiblesCtx = useToggleItems({ scorecard: props.scorecard }) |
There was a problem hiding this comment.
[❗❗ correctness]
The useToggleItems hook is being used with the scorecard prop, which can be of type Scorecard or ScorecardInfo. Ensure that useToggleItems can handle both types correctly, as any mismatch in expected properties could lead to runtime errors.
| [id]: typeof toggle === 'boolean' ? toggle : !prevItems[id], | ||
| })) | ||
| }, []) | ||
| const reviewFormCtx = useReviewForm({ |
There was a problem hiding this comment.
[❗❗ correctness]
The useReviewForm hook is being initialized with reviewItems derived from reviewInfo?.reviewItems or aiFeedbackItems. Ensure that these arrays are compatible with the expected input of useReviewForm, as discrepancies could cause unexpected behavior.
| toggledItems, | ||
| toggleItem, | ||
| doDeleteAppeal: props.doDeleteAppeal, | ||
| form: props.isEdit ? reviewFormCtx.form : undefined, |
There was a problem hiding this comment.
[maintainability]
The formErrors and formTrigger are conditionally set based on isEdit. If isEdit changes dynamically, ensure that the component can handle the transition without causing errors or inconsistent states.
| } | ||
|
|
||
| export const useScorecardContext = (): ScorecardViewerContextValue => useContext(ScorecardViewerContext) | ||
| export const useScorecardViewerContext = (): ScorecardViewerContextValue => useContext(ScorecardViewerContext) |
There was a problem hiding this comment.
[❗❗ correctness]
The function useScorecardViewerContext was renamed. Ensure that all references to the old function name useScorecardContext are updated throughout the codebase to prevent undefined errors.
| } | ||
|
|
||
| .errorTop { | ||
| margin-bottom: $sp-3; |
There was a problem hiding this comment.
[maintainability]
The use of $sp-3 for margin values in .errorTop and .errorBottom assumes that $sp-3 is defined and consistent with the design system. Ensure that $sp-3 is defined in the imported styles and is the intended spacing value.
| const isCompleted = props.reviewInfo?.status === 'COMPLETED' | ||
| const score = isCompleted ? props.reviewInfo!.finalScore! : totalScore | ||
| let status: 'passed' |'failed-score' |'pending' = ( | ||
| score >= (props.scorecard.minimumPassingScore ?? 50) ? 'passed' : 'failed-score' |
There was a problem hiding this comment.
[correctness]
The use of props.scorecard.minimumPassingScore ?? 50 assumes a default minimum passing score of 50 if minimumPassingScore is undefined. Ensure that this default value aligns with business requirements or consider making it configurable.
| </i> | ||
| {errorMessage} | ||
| {' '} | ||
| Check bellow. |
There was a problem hiding this comment.
[💡 readability]
Typo in the error message: 'Check bellow.' should be 'Check below.'
| </i> | ||
| {errorMessage} | ||
| {' '} | ||
| Check above. |
There was a problem hiding this comment.
[💡 readability]
Typo in the error message: 'Check above.' should be 'Check below.'
| } | ||
| }, [totalScore, props.scorecard]) | ||
|
|
||
| if (props.isLoading) { |
There was a problem hiding this comment.
[💡 design]
The isLoading check returns a TableLoading component, but this might not be the most appropriate loading indicator for this context. Ensure that TableLoading is suitable for the ScorecardViewer or consider using a more generic loading indicator.
| }: UseProgressCalculationProps): UseProgressCalculationValue => { | ||
| // Watch form values to automatically recalculate progress | ||
| const watchedReviews = useWatch({ | ||
| control: form?.control, |
There was a problem hiding this comment.
[❗❗ correctness]
The useWatch hook is used with an optional form object. If form is undefined, useWatch will receive undefined for the control property, which may lead to unexpected behavior. Consider adding a check to ensure form is defined before using useWatch.
|
|
||
| const recalculateReviewProgress = useCallback(() => { | ||
| const reviewFormDatas = form?.getValues()?.reviews ?? [] | ||
| return calculateProgressAndScore(reviewFormDatas, scorecard) |
There was a problem hiding this comment.
[❗❗ correctness]
The recalculateReviewProgress function uses form?.getValues()?.reviews, which will return an empty array if form is undefined. This might not be the intended behavior. Ensure that form is always defined when calling this function or handle the case where form is undefined more explicitly.
| ...recalculateReviewProgress(), | ||
| recalculateReviewProgress, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }), [watchedReviews, recalculateReviewProgress]) |
There was a problem hiding this comment.
[performance]
The useMemo dependency array includes recalculateReviewProgress, which is a function created using useCallback. Ensure that all dependencies of recalculateReviewProgress are stable or explicitly listed to avoid unnecessary recalculations.
| ) : [], | ||
| id: reviewItem.id, | ||
| index: reviewItemIndex, | ||
| initialAnswer: ( |
There was a problem hiding this comment.
[❗❗ correctness]
The initialAnswer is cast to a string using as string, but it might be undefined if none of the conditions are met. Consider ensuring that initialAnswer is always a string or handle the undefined case explicitly to avoid potential runtime errors.
| const formData = getValues() | ||
| const isTouchedAll: { [key: string]: boolean } = {} | ||
| forEach(formData.reviews, review => { | ||
| isTouchedAll[`reviews.${review.index}.initialAnswer.message`] = true |
There was a problem hiding this comment.
[❗❗ correctness]
The key reviews.${review.index}.initialAnswer.message seems incorrect as initialAnswer is not an object with a message property. Verify the intended key structure for marking fields as touched.
| } | ||
|
|
||
| export const useToggleItems = (props: UseToggleItemsProps): UseToggleItemsValue => { | ||
| const [toggledItems, setToggledItems] = useState<{[key: string]: boolean}>({}) |
There was a problem hiding this comment.
[performance]
Consider using a Map instead of an object for toggledItems to improve performance when dealing with a large number of items. Map provides better performance for frequent additions and deletions.
| const [toggledItems, setToggledItems] = useState<{[key: string]: boolean}>({}) | ||
|
|
||
| const toggleItem = useCallback((id: string, toggle?: boolean) => { | ||
| setToggledItems(prevItems => ({ |
There was a problem hiding this comment.
[❗❗ correctness]
The toggleItem function mutates the state directly based on the id key. Ensure that id is unique and consistent across the application to avoid unexpected behavior.
| && !!initialAnswer | ||
| ) { | ||
| const totalPoint = question.scaleMax - question.scaleMin | ||
| const initialAnswerNumber = parseInt(initialAnswer as string, 10) - question.scaleMin |
There was a problem hiding this comment.
[❗❗ correctness]
Consider checking if initialAnswer is a valid number before using parseInt. If initialAnswer is not a valid number, parseInt will return NaN, which could lead to unexpected results in the calculation.
| [scorecardQuestionId: string]: string | number | ||
| } = {} | ||
|
|
||
| const newReviewProgress = Math.round( |
There was a problem hiding this comment.
[maintainability]
The mappingResult object is defined but not used effectively in the calculation of scores. Consider revisiting its purpose or removing it if unnecessary to improve code clarity.
| /** | ||
| * Calculate progress and score from review form data | ||
| */ | ||
| export const calculateProgressAndScore = ( |
There was a problem hiding this comment.
[readability]
The function calculateProgressAndScore has a complex nested structure with multiple levels of reduce. Consider refactoring to simplify the logic and improve readability.
| <Link | ||
| className={styles.respondButton} | ||
| to={getReviewRoute(reviewId)} | ||
| to={getReviewRoute(submission.id, reviewId)} |
There was a problem hiding this comment.
[❗❗ correctness]
The change from getReviewRoute(reviewId) to getReviewRoute(submission.id, reviewId) assumes that submission.id is always defined and valid. Ensure that submission.id is not undefined or null to prevent potential runtime errors.
| return ( | ||
| <Link | ||
| to={`./../review/${reviewId}`} | ||
| to={`./../reviews/${data.id}?reviewId=${reviewId}`} |
There was a problem hiding this comment.
[❗❗ security]
The change in the URL path from ./../review/${reviewId} to ./../reviews/${data.id}?reviewId=${reviewId} introduces a potential issue if data.id or reviewId are not properly validated or sanitized. Ensure that these values are safe and cannot be manipulated to access unauthorized resources.
| <Link | ||
| key='complete-review' | ||
| to={`./../review/${reviewId}`} | ||
| to={`./../reviews/${submission.id}?reviewId=${reviewId}`} |
There was a problem hiding this comment.
[❗❗ correctness]
The change in the URL path from ./../review/${reviewId} to ./../reviews/${submission.id}?reviewId=${reviewId} could potentially break existing routes if the new path is not correctly handled elsewhere in the application. Ensure that the new route is supported and that any dependencies on the old path are updated accordingly.
| } | ||
|
|
||
| const reviewUrl = getReviewUrl ? getReviewUrl(reviewId) : getReviewRoute(reviewId) | ||
| const reviewUrl = getReviewUrl ? getReviewUrl(reviewId) : getReviewRoute(submission.id, reviewId) |
There was a problem hiding this comment.
[❗❗ correctness]
The change from getReviewRoute(reviewId) to getReviewRoute(submission.id, reviewId) modifies the function call signature. Ensure that getReviewRoute is designed to handle two parameters and that this change is intentional and tested. This could impact routing logic if not handled correctly.
| } | ||
|
|
||
| const reviewUrl = getReviewUrl ? getReviewUrl(reviewId) : getReviewRoute(reviewId) | ||
| const reviewUrl = getReviewUrl ? getReviewUrl(reviewId) : getReviewRoute(submission.id, reviewId) |
There was a problem hiding this comment.
[❗❗ correctness]
Similar to the change on line 588, ensure that getReviewRoute is intended to accept two parameters. This change should be verified to ensure it does not introduce routing errors.
|
|
||
| export function useFetchAiWorkflowsRunItems( | ||
| workflowId: string, | ||
| workflowId: string | undefined, |
There was a problem hiding this comment.
[❗❗ correctness]
Allowing workflowId and runId to be undefined without handling this case in the SWR key can lead to unexpected behavior. Consider providing a default value or ensuring these are always defined before use.
| * @returns reviews info | ||
| */ | ||
| export function useFetchSubmissionReviews(): useFetchSubmissionReviewsProps { | ||
| export function useFetchSubmissionReviews(reviewId: string = ''): useFetchSubmissionReviewsProps { |
There was a problem hiding this comment.
[❗❗ correctness]
The reviewId parameter is now passed directly to the useFetchSubmissionReviews function instead of being retrieved from useParams. Ensure that all components using this hook are updated to pass the reviewId explicitly, as this change could break existing functionality if not handled correctly.
| isValidating: isLoadingSubmission, | ||
| }: SWRResponse<BackendSubmission, Error> = useSWR<BackendSubmission, Error>( | ||
| `EnvironmentConfig.API.V6/submissions/${submissionId}`, | ||
| `/submissions/${submissionId}`, |
There was a problem hiding this comment.
[❗❗ correctness]
The URL for fetching submissions has been changed from EnvironmentConfig.API.V6/submissions/${submissionId} to /submissions/${submissionId}. Ensure that the base URL is correctly configured elsewhere, as this change assumes that the base URL is set globally or in a higher context.
| } | ||
|
|
||
| export interface ReviewsContextModel extends ChallengeDetailContextModel { | ||
| isLoading: boolean |
There was a problem hiding this comment.
[💡 style]
Consider adding a semicolon at the end of the isLoading property for consistency with the rest of the interface properties.
| export interface ReviewsContextModel extends ChallengeDetailContextModel { | ||
| isLoading: boolean | ||
| reviewId?: string; | ||
| submissionId: string |
There was a problem hiding this comment.
[design]
Consider making submissionId optional if there are valid scenarios where a ReviewsContextModel might not have a submissionId. Otherwise, ensure that this field is always provided to avoid potential runtime errors.
| workflow?: AiWorkflow | ||
| workflowRun?: AiWorkflowRun | ||
| scorecard?: Scorecard | ||
| workflowRuns: AiWorkflowRun[] |
There was a problem hiding this comment.
[performance]
If workflowRuns can be a large array, consider using a more efficient data structure or pagination to handle potential performance issues.
| * Builds the review detail route for the current challenge context. | ||
| */ | ||
| export function getReviewRoute( | ||
| submissionId: string, |
There was a problem hiding this comment.
[security]
The submissionId parameter is added but not validated. Consider adding validation to ensure it is a valid and expected format to prevent potential issues with malformed URLs.
| submissionInfo, | ||
| saveReviewInfo, | ||
| }: useFetchSubmissionReviewsProps = useFetchSubmissionReviews() | ||
| }: useFetchSubmissionReviewsProps = useFetchSubmissionReviews(reviewId) |
There was a problem hiding this comment.
[❗❗ correctness]
Passing reviewId to useFetchSubmissionReviews changes its behavior. Ensure that reviewId is always defined and valid to avoid potential runtime errors.
| workflowId: string, | ||
| }>() | ||
| const [searchParams] = useSearchParams() | ||
| const workflowId = searchParams.get('workflowId') ?? '' |
There was a problem hiding this comment.
[security]
Using useSearchParams to extract workflowId and reviewId from the URL query parameters is correct, but consider validating these values to ensure they meet expected formats or constraints. This can help prevent potential issues if unexpected values are passed in the URL.
| const workflowId = searchParams.get('workflowId') ?? '' | ||
| const reviewId = searchParams.get('reviewId') ?? '' | ||
|
|
||
| const [reviewStatus, setReviewStatus] = useState({} as ReviewCtxStatus) |
There was a problem hiding this comment.
[correctness]
The initial state for reviewStatus is set to an empty object cast as ReviewCtxStatus. Ensure that this initial state is appropriate for all cases where reviewStatus is used, as it might lead to runtime errors if properties are accessed without being defined.
kkartunov
left a comment
kkartunov
left a comment
There was a problem hiding this comment.
Looks very clean. Awesome PR!
Few minor observations. Ready to merge when those updated...
| isValidating: isLoadingSubmission, | ||
| }: SWRResponse<BackendSubmission, Error> = useSWR<BackendSubmission, Error>( | ||
| `EnvironmentConfig.API.V6/submissions/${submissionId}`, | ||
| `/submissions/${submissionId}`, |
There was a problem hiding this comment.
This correct url we are feeding to SWR?
There was a problem hiding this comment.
This is just the swr key - the one used for cache. The fetcher composes the url by itself.
src/apps/review/src/lib/components/Scorecard/ScorecardViewer/ScorecardGroup/ScorecardGroup.tsx
Outdated
Show resolved
Hide resolved
src/apps/review/src/lib/components/Scorecard/ScorecardViewer/ScorecardGroup/ScorecardGroup.tsx
Outdated
Show resolved
Hide resolved
| </p> | ||
| )} | ||
| {feedback.content} | ||
| <MarkdownReview value={feedback.content} /> |
There was a problem hiding this comment.
@vas3a very nice - Markdown support for AI generated feedback, right?!
|
|
||
| return ( | ||
| <div className={styles.wrap}> | ||
| <div className={styles.wrap} onClick={stopPropagation}> |
There was a problem hiding this comment.
[design]
The onClick handler stopPropagation is added to the div wrapping the table. Ensure that this behavior is intended, as it will prevent any click events from bubbling up from the table. If this is not the desired behavior, consider removing it or applying it more selectively.
| const { scoreMap, toggleItem, toggledItems }: ScorecardViewerContextValue = useScorecardViewerContext() | ||
|
|
||
| const isVissible = !toggledItems[props.group.id] | ||
| const isVisible = !toggledItems[props.group.id] |
There was a problem hiding this comment.
[correctness]
Typo fix: isVissible was corrected to isVisible. Ensure all references to this variable are updated consistently across the codebase to prevent any potential issues.
| </div> | ||
| )} | ||
|
|
||
| {totalScore && !!props.aiFeedback && ( |
There was a problem hiding this comment.
[❗❗ correctness]
The condition totalScore && !!props.aiFeedback may lead to unexpected behavior if totalScore is 0, as it will evaluate to false. Consider using totalScore !== undefined to ensure the check is only for undefined values.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-2178
What's in this PR?
Merge scorecard review with updated AI workflows UI
Not sure if this is the best approach, but there was too much effort to make the existing UI fit the new UI. So I Re-worked the scorcard viewer into a new set of components so I can make them fit the AI workflow scrocards and the existing scorecards.
The result is a mashup between what we had and some addition.
I also used a bit of help from cursor to initially port over the existing code. It did mostly good but I had a lot of rewrite after it. There might be hiccups/misses. I tested everything I could, but it's a lot of area to cover.
Also, I haven't removed the existing scorecard ui. It is still accessible via the existing route (/review/:reviewId). This is so we can compare the new UI with the existing ones, and make sure it's still behaving the same.
The UI will point by default to the new scorcard viewer.