Feat/ai workflows #1294
Feat/ai workflows #1294
Conversation
…anner PM-2133 dismissable banner
…to feat/ai-workflows
…nges-page PM-1904 active challenges page
…s-page PM-1905 - ai reviews
…browsing-ui PM-1906 ai scorecard browsing UI
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| {run.status === 'SUCCESS' ? ( | ||
| run.workflow.scorecard ? ( | ||
| <a | ||
| href={`./ai-scorecard/${props.submission.id}/${run.workflow.id}`} |
There was a problem hiding this comment.
[maintainability]
The relative URL ./ai-scorecard/${props.submission.id}/${run.workflow.id} could lead to unexpected behavior if the component is used in a different routing context. Consider using an absolute path or a routing helper to ensure the link works correctly across different parts of the application.
| } | ||
|
|
||
| const ScorecardAttachments: FC<ScorecardAttachmentsProps> = props => ( | ||
| <div className={props.className}> |
There was a problem hiding this comment.
[correctness]
Consider providing a default value for className to ensure that the div element always has a valid class attribute, which can prevent potential rendering issues.
| transition: 0.15s ease-in-out; | ||
|
|
||
| &:hover { | ||
| background: darken(#00797A, 1.5%); |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a CSS variable for the color #00797A to improve maintainability. This will make it easier to update the color scheme in the future if needed.
| } | ||
|
|
||
| &:active { | ||
| transition: none; |
There was a problem hiding this comment.
[💡 design]
The transition: none; on &:active might cause a jarring effect when transitioning from :hover to :active. Consider maintaining a consistent transition effect for better user experience.
| } | ||
| } | ||
|
|
||
| @include ltemd { |
There was a problem hiding this comment.
[correctness]
Ensure that the @include ltemd mixin is defined and behaves as expected across different screen sizes. If this mixin is not well-documented, it might lead to unexpected layout issues.
| const allFeedbackItems = aiFeedbackItems || [] | ||
| const { toggleItem, toggledItems }: ScorecardViewerContextValue = useScorecardContext() | ||
|
|
||
| const isVissible = !toggledItems[props.group.id] |
There was a problem hiding this comment.
[💡 readability]
There is a typo in the variable name isVissible. It should be isVisible. This could lead to confusion and should be corrected for clarity.
| aiFeedbackItems?.find(r => r.scorecardQuestionId === props.question.id) | ||
| ), [props.question.id, aiFeedbackItems]) | ||
|
|
||
| if (!aiFeedbackItems?.length || !feedback) { |
There was a problem hiding this comment.
[correctness]
Returning an empty fragment (<></>) when there is no feedback might be misleading if the parent component expects a specific structure or behavior. Consider returning null instead to explicitly indicate the absence of a component.
| const AiFeedback: FC<AiFeedbackProps> = props => { | ||
| const { aiFeedbackItems }: ScorecardViewerContextValue = useScorecardContext() | ||
| const feedback = useMemo(() => ( | ||
| aiFeedbackItems?.find(r => r.scorecardQuestionId === props.question.id) |
There was a problem hiding this comment.
[💡 performance]
The use of useMemo here might not be necessary since the computation inside is a simple find operation. If aiFeedbackItems or props.question.id change frequently, the memoization overhead might outweigh the benefits. Consider removing useMemo unless performance profiling indicates a need for it.
| @@ -0,0 +1,25 @@ | |||
| @import '@libs/ui/styles/includes'; | |||
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that the imported file @libs/ui/styles/includes exists and is correctly resolved in the build process. Missing or incorrect imports can lead to build failures or runtime errors.
| display: block; | ||
| width: 16px; | ||
| height: 16px; | ||
| color: #767676; |
There was a problem hiding this comment.
[maintainability]
Consider using a CSS variable or a SCSS variable for the color #767676 to maintain consistency and ease future updates across the application.
| .questionText { | ||
| font-weight: bold; | ||
| + * { | ||
| margin-top: $sp-2; |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that the variable $sp-2 is defined in the imported styles. Undefined variables can cause compilation errors.
| const ScorecardQuestion: FC<ScorecardQuestionProps> = props => { | ||
| const { toggleItem, toggledItems }: ScorecardViewerContextValue = useScorecardContext() | ||
|
|
||
| const isToggled = toggledItems[props.question.id!] |
There was a problem hiding this comment.
[❗❗ correctness]
Using non-null assertion (!) on props.question.id could lead to runtime errors if id is ever null or undefined. Consider adding a check to ensure id is defined before using it.
| const { toggleItem, toggledItems }: ScorecardViewerContextValue = useScorecardContext() | ||
|
|
||
| const isToggled = toggledItems[props.question.id!] | ||
| const toggle = useCallback(() => toggleItem(props.question.id!), [props.question, toggleItem]) |
There was a problem hiding this comment.
[performance]
The dependency array for useCallback should include props.question.id instead of props.question to avoid unnecessary re-creations of the toggle function when other properties of props.question change.
|
|
||
| .icon { | ||
| width: 24px; | ||
| flex: 0 0 auto |
There was a problem hiding this comment.
[❗❗ correctness]
Missing semicolon at the end of the flex property. This could lead to unexpected behavior in some browsers.
|
|
||
| .index { | ||
| width: 128px; | ||
| flex: 0 0 auto |
There was a problem hiding this comment.
[❗❗ correctness]
Missing semicolon at the end of the flex property. This could lead to unexpected behavior in some browsers.
| interface ScorecardQuestionRowProps extends PropsWithChildren { | ||
| className?: string | ||
| icon?: ReactNode | ||
| index?: string |
There was a problem hiding this comment.
[correctness]
Consider using a more specific type than string for index if possible. This could improve type safety and prevent potential errors if index is expected to be a number or a specific format.
| @@ -0,0 +1,15 @@ | |||
| @import '@libs/ui/styles/includes'; | |||
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that the @libs/ui/styles/includes path is correct and that the imported file exists. Incorrect paths can lead to build failures.
| opacity: 0.75; | ||
| } | ||
|
|
||
| @include ltemd { |
There was a problem hiding this comment.
[❗❗ correctness]
Verify that the ltemd mixin is defined in the imported styles. If it's missing, it will cause a compilation error.
| } | ||
|
|
||
| export const calcScore = (score: number, scaleMax: number, weight: number): number => ( | ||
| (score / (scaleMax || 1)) * weight |
There was a problem hiding this comment.
[correctness]
Using scaleMax || 1 to avoid division by zero can mask potential errors where scaleMax is unexpectedly zero. Consider handling this case explicitly to ensure that scaleMax is validated before calling calcScore.
| </strong> | ||
| <span>/</span> | ||
| <span> | ||
| {props.weight.toFixed(2)} |
There was a problem hiding this comment.
[correctness]
Using toFixed(2) on props.weight assumes that weight is always a valid number. Consider adding validation or error handling to ensure weight is a valid number before formatting.
| line-height: 20px; | ||
| color: #0A0A0A; | ||
|
|
||
| padding-right: 56px; |
There was a problem hiding this comment.
[maintainability]
The use of padding-right: 56px; alongside padding: $sp-4; may lead to unexpected layout issues due to conflicting padding values. Consider consolidating padding values for consistency.
| .score { | ||
| order: 7; | ||
| width: 100%; | ||
| margin-left: $sp-10; |
There was a problem hiding this comment.
[maintainability]
The use of margin-left: $sp-10; on .score within a media query might lead to inconsistent spacing across different screen sizes. Ensure this margin is necessary and doesn't conflict with other layout rules.
|
|
||
| const score = useMemo(() => ( | ||
| calcSectionScore(props.section, allFeedbackItems) | ||
| ), [props.section, allFeedbackItems]) |
There was a problem hiding this comment.
[correctness]
Consider adding props.section.questions to the dependency array of useMemo. If the questions change, the score might need to be recalculated.
| align-items: center; | ||
| gap: $sp-6; | ||
|
|
||
| padding: $sp-4 56px; |
There was a problem hiding this comment.
[maintainability]
The use of a hardcoded pixel value (56px) for padding may lead to layout issues on different screen sizes. Consider using a responsive unit or a variable from your design system for consistency and maintainability.
| import styles from './ScorecardTotal.module.scss' | ||
|
|
||
| interface ScorecardTotalProps { | ||
| score?: number |
There was a problem hiding this comment.
[💡 maintainability]
Consider explicitly specifying the default value for score in the ScorecardTotalProps interface to improve clarity and maintainability.
| <strong>Total Score</strong> | ||
| <span className={styles.mx} /> | ||
| <ScorecardScore | ||
| score={props.score ?? 0} |
There was a problem hiding this comment.
[correctness]
Using the nullish coalescing operator ?? is appropriate here to provide a default value of 0 for score. However, ensure that ScorecardScore can handle a score of 0 correctly.
| toggleItem: (id: string) => void | ||
| }; | ||
|
|
||
| const ScorecardViewerContext = createContext({} as ScorecardViewerContextValue) |
There was a problem hiding this comment.
[❗❗ correctness]
The createContext is initialized with an empty object cast to ScorecardViewerContextValue. This can lead to runtime errors if any component tries to access properties before they are set. Consider providing a default value that matches the expected shape of ScorecardViewerContextValue.
| export const ScorecardViewerContextProvider: FC<ScorecardViewerContextProps> = props => { | ||
| const [toggledItems, setToggledItems] = useState<{[key: string]: boolean}>({}) | ||
|
|
||
| const toggleItem = useCallback((id: string, toggle?: boolean) => { |
There was a problem hiding this comment.
[maintainability]
The toggleItem function allows an optional toggle parameter, but this parameter is not documented in the ScorecardViewerContextValue type definition. Consider updating the type definition to reflect the actual usage of the function.
| @@ -0,0 +1,27 @@ | |||
| @import '@libs/ui/styles/includes'; | |||
There was a problem hiding this comment.
[maintainability]
Consider using a more specific import path instead of @libs/ui/styles/includes if possible. This can help reduce the risk of importing unnecessary styles and improve maintainability.
|
|
||
| .conclusion { | ||
| padding: $sp-4; | ||
| background: #E0E4E8; |
There was a problem hiding this comment.
[maintainability]
The background color #E0E4E8 is hardcoded. Consider using a variable from your styles includes if available, to maintain consistency and ease future changes.
| .conclusion { | ||
| padding: $sp-4; | ||
| background: #E0E4E8; | ||
| color: #0A0A0A; |
There was a problem hiding this comment.
[maintainability]
The text color #0A0A0A is hardcoded. Consider using a variable from your styles includes if available, to maintain consistency and ease future changes.
| scorecard={props.scorecard} | ||
| aiFeedbackItems={props.aiFeedback} | ||
| > | ||
| {!!props.score && ( |
There was a problem hiding this comment.
[❗❗ correctness]
Using !!props.score to check for the presence of a score might lead to unexpected behavior if score is 0. Consider using a more explicit check, such as props.score !== undefined.
| Congratulations! You earned a score of | ||
| {' '} | ||
| <strong> | ||
| {props.score.toFixed(2)} |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure props.score is defined before calling toFixed(2), as calling it on undefined will throw an error. Consider using optional chaining or a default value.
| out of the maximum of | ||
| {' '} | ||
| <strong> | ||
| {props.scorecard.maxScore.toFixed(2)} |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure props.scorecard.maxScore is defined before calling toFixed(2), as calling it on undefined will throw an error. Consider using optional chaining or a default value.
|
|
||
| return section.questions.reduce((sum, question) => ( | ||
| sum + ( | ||
| (feedbackItemsMap[question.id as string]?.questionScore ?? 0) / (question.scaleMax || 1) |
There was a problem hiding this comment.
[❗❗ correctness]
Using question.id as string assumes that question.id is always a string. If question.id can be of another type, this could lead to runtime errors. Consider ensuring question.id is always a string or handle other types appropriately.
|
|
||
| return section.questions.reduce((sum, question) => ( | ||
| sum + ( | ||
| (feedbackItemsMap[question.id as string]?.questionScore ?? 0) / (question.scaleMax || 1) |
There was a problem hiding this comment.
[❗❗ correctness]
The division by (question.scaleMax || 1) could lead to unexpected results if question.scaleMax is 0. Consider validating question.scaleMax to ensure it is never 0 or handle the case explicitly.
| ): number => ( | ||
| group.sections.reduce((sum, section) => ( | ||
| sum + ( | ||
| calcSectionScore(section, feedbackItems) |
There was a problem hiding this comment.
[performance]
The calcSectionScore function is called within a loop for each section in calcGroupScore. If calcSectionScore is computationally expensive, consider optimizing or memoizing results if possible.
| error: fetchError, | ||
| isValidating: isLoading, | ||
| }: SWRResponse<AiWorkflowRunItem[], Error> = useSWR<AiWorkflowRunItem[], Error>( | ||
| `${TC_API_BASE_URL}/workflows/${workflowId}/runs/${runId}/items`, |
There was a problem hiding this comment.
[❗❗ correctness]
The useSWR hook is being used with a URL that includes runId directly. If runId is undefined, this will result in a malformed URL. Consider adding a check to ensure runId is defined before constructing the URL.
| }: SWRResponse<AiWorkflowRunItem[], Error> = useSWR<AiWorkflowRunItem[], Error>( | ||
| `${TC_API_BASE_URL}/workflows/${workflowId}/runs/${runId}/items`, | ||
| { | ||
| isPaused: () => !workflowId || !runId, |
There was a problem hiding this comment.
[❗❗ correctness]
The isPaused function in useSWR checks for !workflowId || !runId, which is good for pausing the request. However, if runId is undefined, the URL will still be constructed with undefined as part of it. Consider restructuring to avoid constructing the URL when runId is not defined.
| export interface AiFeedbackItem { | ||
| id: string | ||
| content: string | ||
| upVotes: number |
There was a problem hiding this comment.
[correctness]
Consider using number type for upVotes and downVotes only if negative values are not expected. If negative values are possible, consider using a more appropriate type or adding validation.
| content: string | ||
| upVotes: number | ||
| downVotes: number | ||
| questionScore: number |
There was a problem hiding this comment.
[correctness]
Ensure that questionScore is validated to be within an expected range, as incorrect values could impact the logic that relies on this score.
| flex-direction: column; | ||
| } | ||
|
|
||
| .pageContentWrap { |
There was a problem hiding this comment.
[maintainability]
Renaming .contentWrap to .pageContentWrap improves specificity, but ensure that all references to .contentWrap in the codebase are updated to prevent any broken styles.
| const AiScorecardViewer: FC = () => { | ||
| const { showBannerNotification, removeNotification }: NotificationContextType = useNotification() | ||
| const { challengeInfo, scorecard, workflowId, workflowRun }: AiScorecardContextModel = useAiScorecardContext() | ||
| const { runItems }: AiWorkflowRunItemsResponse = useFetchAiWorkflowsRunItems(workflowId, workflowRun?.id) |
There was a problem hiding this comment.
[correctness]
Consider handling the case where useFetchAiWorkflowsRunItems might return an error or undefined runItems. This could prevent potential runtime errors if runItems is used without validation.
| [], | ||
| ) | ||
|
|
||
| const [selectedTab, setSelectedTab] = useState('scorecard') |
There was a problem hiding this comment.
[💡 design]
The initial state for selectedTab is hardcoded as 'scorecard'. Consider making this configurable or deriving it from a prop if there's a possibility that the initial tab might change based on different conditions.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/
What's in this PR?
AI Workflows work