Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ev.stopPropagation() | ||
| } | ||
|
|
||
| function normalizeStatus( |
There was a problem hiding this comment.
[maintainability]
The normalizeStatus function has a cyclomatic complexity that might be higher than necessary. Consider refactoring to simplify the logic and improve maintainability.
| [runs], | ||
| ) | ||
|
|
||
| const reviewerRows = useMemo<AiReviewerRow[]>(() => { |
There was a problem hiding this comment.
[maintainability]
The reviewerRows useMemo hook contains a lot of logic, including filtering, mapping, and conditionals. Consider breaking this into smaller functions to improve readability and maintainability.
| [reviewerRows], | ||
| ) | ||
|
|
||
| const lockMessage = useMemo(() => { |
There was a problem hiding this comment.
[💡 readability]
The lockMessage useMemo hook constructs a message using string concatenation. Consider using template literals for better readability.
| + 'Improve your submission and resubmit.' | ||
| }, [currentDecision?.submissionLocked, failedGatingReviewers]) | ||
|
|
||
| if (isTablet) { |
There was a problem hiding this comment.
[maintainability]
The conditional rendering logic for the tablet view is complex and repeated for both mobile and desktop views. Consider extracting this logic into a separate component to avoid duplication and improve maintainability.
|
|
||
| const aiReviewersCount = useMemo(() => { | ||
| const reviewersCount = props.aiReviewers.length || aiReviewConfig?.workflows?.length || 0 | ||
| return reviewersCount + 1 |
There was a problem hiding this comment.
[correctness]
The calculation of aiReviewersCount assumes that either props.aiReviewers.length or aiReviewConfig?.workflows?.length is non-zero. If both are zero, this will incorrectly return 1. Consider handling this case explicitly to avoid potential confusion or errors.
| </span> | ||
| </Tooltip> | ||
| )} | ||
| {formatScore(currentDecision!.totalScore)} |
There was a problem hiding this comment.
[correctness]
The use of the non-null assertion operator (!) on currentDecision!.totalScore assumes that currentDecision is always defined when hasScore is true. Ensure that this assumption holds in all cases, or consider adding a check to prevent potential runtime errors.
| 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.
[correctness]
The calculation of aiReviewersCount uses a fallback value of 1 if both aiReviewers.length and aiReviewConfig?.workflows?.length are 0. This might lead to incorrect counts if there are no reviewers or workflows. Consider revising the logic to accurately reflect the count of reviewers.
| {aiReviewersCount > 0 && ( | ||
| <> | ||
| <td className={styles.cellVirusScan}> | ||
| {hasDecisionScore ? formatScore(currentDecision!.totalScore) : '-'} |
There was a problem hiding this comment.
[❗❗ correctness]
Using the non-null assertion operator (!) on currentDecision could lead to runtime errors if currentDecision is undefined. Consider adding a check to ensure currentDecision is defined before accessing totalScore.
| {toggledRows.has(submission.id) && ( | ||
| <tr> | ||
| <td className={styles.aiReviewersTableRow} colSpan={4}> | ||
| <td className={styles.aiReviewersTableRow} colSpan={aiReviewersCount ? 6 : 4}> |
There was a problem hiding this comment.
[maintainability]
The colSpan attribute is dynamically set based on aiReviewersCount. Ensure that aiReviewersCount accurately reflects the number of columns to avoid layout issues.
| 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. If aiReviewConfig is undefined, this will pass undefined to the hook, which may not be the intended behavior. Consider adding a check to ensure aiReviewConfig is defined before calling the hook.
| }: 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 in useMemo does not handle the case where aiReviewDecisions might be undefined or null. Ensure that aiReviewDecisions is always an array before calling reduce to prevent runtime errors.
| try { | ||
| return await fetchAiReviewConfig(challengeId) | ||
| } catch (fetchError) { | ||
| if (isNotFoundError(fetchError)) { |
There was a problem hiding this comment.
[💡 maintainability]
Consider logging the error details before returning undefined for a 404 error. This could help in debugging and understanding why a specific request was not found.
| { | ||
| fetcher: async (): Promise<AiReviewDecision[]> => { | ||
| if (!configId) { | ||
| return [] |
There was a problem hiding this comment.
[correctness]
Consider handling potential errors in fetchAiReviewDecisions similar to how errors are handled in fetchAiReviewConfig. This would ensure consistency and robustness in error handling across different fetch operations.
| id: string | ||
| challengeId: string | ||
| version: number | ||
| minPassingThreshold: number |
There was a problem hiding this comment.
[💡 readability]
Consider using a more descriptive type for minPassingThreshold to clarify its intended range and constraints, such as a custom type or a more specific numeric type.
| challengeId: string | ||
| version: number | ||
| minPassingThreshold: number | ||
| mode: string |
There was a problem hiding this comment.
[correctness]
The mode field is a string, which might be prone to errors if not validated. Consider using a union type or an enum to restrict possible 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 field is a string, which could lead to inconsistencies if not properly validated. Consider using a union type or an enum to define the possible statuses.
|
|
||
| export interface AiReviewDecisionBreakdown { | ||
| evaluatedAt?: string | ||
| mode?: string |
There was a problem hiding this comment.
[correctness]
The mode field is optional and a string. Consider using a union type or an enum to define acceptable values, ensuring consistency and reducing potential errors.
| 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 AI review decisions might not be available. This would align with the optional nature of aiReviewConfig and improve the model's flexibility.
|
|
||
| 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 string. This will prevent potential 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]
Ensure that challengeId is validated before being used in the URL. If challengeId is undefined or an empty string, it might lead to unexpected API behavior.
| `${v6BaseUrl}/ai-review/decisions?configId=${configId ?? ''}` | ||
| ) | ||
|
|
||
| export const fetchAiReviewDecisions = async (configId: string): Promise<AiReviewDecision[]> => ( |
There was a problem hiding this comment.
[maintainability]
Consider adding error handling for the xhrGetAsync call to manage potential network or API errors gracefully.
| @@ -1,4 +1,5 @@ | |||
| import { FC, Fragment, MouseEvent, useCallback, useMemo, useState } from 'react' | |||
| /* eslint-disable complexity */ | |||
There was a problem hiding this comment.
[maintainability]
Disabling the complexity rule for the entire file can hide potential issues. Consider refactoring complex functions instead of disabling the rule globally.
| = challengeDetailContext.aiReviewDecisionsBySubmissionId | ||
| const currentDecision = aiReviewDecisionsBySubmissionId[submission.id] | ||
| const hasDecisionScore = currentDecision?.totalScore !== null && currentDecision?.totalScore !== undefined | ||
| const normalizedStatus = normalizeDecisionStatus(currentDecision?.status ?? undefined) |
There was a problem hiding this comment.
[correctness]
The change from null to undefined in the normalizeDecisionStatus function call is subtle. Ensure that the function handles undefined as expected, as this could affect the logic if null and undefined are treated differently.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Stale closure in renderHistoryRow: missing challengeDetailContext, aiReviewersCount, and aiReviewers in useCallback dependencies
The renderHistoryRow callback (line 232) reads challengeDetailContext.aiReviewDecisionsBySubmissionId (line 289-291), aiReviewersCount (lines 312, 315, 320), and aiReviewers (line 337), but none of these are included in the dependency array (lines 344-351). Since aiReviewDecisionsBySubmissionId is populated asynchronously from an API response via context, the callback captures the initial empty {} value and never updates when decision data arrives. This causes the Score and Status columns to permanently show - in each history row, even after the data has loaded, until an unrelated dependency like toggledRows changes.
(Refers to lines 344-351)
Was this helpful? React with 👍 or 👎 to provide feedback.
| {toggledRows.has(submission.id) && ( | ||
| <tr> | ||
| <td className={styles.aiReviewersTableRow} colSpan={4}> | ||
| <td className={styles.aiReviewersTableRow} colSpan={aiReviewersCount ? 6 : 4}> |
There was a problem hiding this comment.
🟡 Incorrect colSpan value (6) for expanded row when table only has 5 columns
The expanded row at line 335 uses colSpan={aiReviewersCount ? 6 : 4}. Since aiReviewersCount is always >= 1 (computed as (aiReviewers.length || aiReviewConfig?.workflows?.length || 0) + 1 at SubmissionHistoryModal.tsx:139), this always evaluates to 6. However, the table only has 5 columns: Submission ID, Submitted, Reviewer, Score, and Status (the Score/Status columns are always shown since aiReviewersCount > 0 is always true at line 320/371). The colSpan should be 5, not 6.
| <td className={styles.aiReviewersTableRow} colSpan={aiReviewersCount ? 6 : 4}> | |
| <td className={styles.aiReviewersTableRow} colSpan={aiReviewersCount ? 5 : 3}> |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
Adds AI Review (config + decision) fetching and surfaces AI decision outcomes in the review UI (AI reviews table, collapsible row summary, and submission history modal) by extending the challenge detail context.
Changes:
- Introduces new AI Review models, service calls, and SWR hooks to fetch AI review config and decisions from the V6 API.
- Extends
ChallengeDetailContextto provide AI review config/decisions + loading flags to downstream components. - Updates AI review UI components to render overall AI score/status, gating indicators, weights/min scores, and “submission locked” messaging.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/apps/review/src/lib/services/index.ts | Exposes new AI review service from the services barrel. |
| src/apps/review/src/lib/services/aiReview.service.ts | Adds API calls/cache keys for AI review config and decisions. |
| src/apps/review/src/lib/models/index.ts | Exports new AI Review model types. |
| src/apps/review/src/lib/models/ChallengeDetailContextModel.model.ts | Extends challenge detail context contract with AI review fields. |
| src/apps/review/src/lib/models/AiReview.model.ts | Defines AI review config/decision DTO shapes used by UI and hooks. |
| src/apps/review/src/lib/hooks/useFetchAiReviewData.ts | Adds SWR hooks to fetch AI review config and decisions. |
| src/apps/review/src/lib/hooks/index.ts | Re-exports new AI review hooks. |
| src/apps/review/src/lib/contexts/ChallengeDetailContextProvider.tsx | Fetches AI review config/decisions and places them into context. |
| src/apps/review/src/lib/contexts/ChallengeDetailContext.ts | Adds defaults for new AI review context fields. |
| src/apps/review/src/lib/components/TableSubmissionScreening/TableSubmissionScreening.tsx | Forwards aiReviewers into downstream history/details UI. |
| src/apps/review/src/lib/components/TableReviewForSubmitter/TableReviewForSubmitter.tsx | Forwards aiReviewers into downstream history/details UI. |
| src/apps/review/src/lib/components/TableReview/TableReview.tsx | Forwards aiReviewers into downstream history/details UI. |
| src/apps/review/src/lib/components/SubmissionHistoryModal/SubmissionHistoryModal.tsx | Adds AI score/status columns and uses context decisions/config for display. |
| src/apps/review/src/lib/components/SubmissionHistoryModal/SubmissionHistoryModal.module.scss | Minor styling tweak for vertical alignment. |
| src/apps/review/src/lib/components/CollapsibleAiReviewsRow/CollapsibleAiReviewsRow.tsx | Shows overall AI score/status summary and formula tooltip using context config/decisions. |
| src/apps/review/src/lib/components/CollapsibleAiReviewsRow/CollapsibleAiReviewsRow.module.scss | Adds layout/styling for the new summary header and tooltip. |
| src/apps/review/src/lib/components/AiReviewsTable/AiWorkflowRunStatus.tsx | Extends status prop union to support a “failed” state. |
| src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.tsx | Merges configured workflows, decision breakdown, and workflow runs into a unified reviewer table + gating/lock UI. |
| src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.module.scss | Adds styles for locked banner, gating marker, and row result coloring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| export * from './resources.service' | ||
| export * from './payments.service' | ||
| export * from './challenge-phases.service' | ||
| export * from './aiReview.service' |
There was a problem hiding this comment.
Service filenames in this folder consistently use kebab-case (e.g. challenge-phases.service.ts, file-upload.service.ts). This new export uses ./aiReview.service, which breaks that convention and can also be error-prone on case-sensitive filesystems. Consider renaming the file/export to ai-review.service to match the established pattern.
| export * from './aiReview.service' | |
| export * from './ai-review.service' |
| registrants: BackendResource[] | ||
| reviewers: BackendResource[] | ||
| aiReviewConfig?: AiReviewConfig | ||
| aiReviewDecisionsBySubmissionId: Record<string, AiReviewDecision> |
There was a problem hiding this comment.
aiReviewDecisionsBySubmissionId is typed as Record<string, AiReviewDecision>, but the code later indexes it and expects missing keys (e.g. const currentDecision = ...[submission.id]). With Record, indexed access is typed as always present, which can hide real undefined cases. Prefer Partial<Record<string, AiReviewDecision>> (or Record<string, AiReviewDecision | undefined>) to accurately model optional entries.
| aiReviewDecisionsBySubmissionId: Record<string, AiReviewDecision> | |
| aiReviewDecisionsBySubmissionId: Partial<Record<string, AiReviewDecision>> |
| 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), | ||
| [aiReviewConfig?.workflows?.length, aiReviewers.length], | ||
| ) |
There was a problem hiding this comment.
aiReviewersCount is computed as (reviewersOrWorkflowsCount + 1), so it will always be at least 1 even when there are no AI reviewers and no AI review config. As a result, checks like aiReviewersCount > 0 become effectively unconditional (always true) and the extra Score/Status columns will always render. Consider separating "has AI review columns" from the display count (or only adding +1 at the point where you render the label).
| {toggledRows.has(submission.id) && ( | ||
| <tr> | ||
| <td className={styles.aiReviewersTableRow} colSpan={4}> | ||
| <td className={styles.aiReviewersTableRow} colSpan={aiReviewersCount ? 6 : 4}> |
There was a problem hiding this comment.
The expanded row colSpan doesn't match the number of columns rendered. The table currently renders 3 base columns plus 2 optional Score/Status columns (= 5), but the expanded row uses colSpan={aiReviewersCount ? 6 : 4}, which will typically evaluate to 6 and can break table layout/alignment. Compute colSpan directly from the same condition used to render the extra columns (e.g., 5 vs 3).
| <td className={styles.aiReviewersTableRow} colSpan={aiReviewersCount ? 6 : 4}> | |
| <td className={styles.aiReviewersTableRow} colSpan={aiReviewersCount > 0 ? 5 : 3}> |
| 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) |
There was a problem hiding this comment.
These AI decision/config reads are inside renderHistoryRow, which is memoized with useCallback. The callback’s dependency list (later in the file) does not include challengeDetailContext / AI decisions or aiReviewersCount, so this row renderer can capture stale context values and not update when SWR/context data changes. Include the missing dependencies or avoid memoizing this renderer.
| if (status === 'ERROR') { | ||
| return 'failed' | ||
| } | ||
|
|
There was a problem hiding this comment.
This function defaults to returning pending for any unhandled status. Since AiReviewDecisionStatus also includes HUMAN_OVERRIDE, that value would currently be displayed as pending. Please add an explicit mapping for HUMAN_OVERRIDE (and consider a clearer fallback for truly unknown statuses).
| if (status === 'HUMAN_OVERRIDE') { | |
| // Explicitly handle HUMAN_OVERRIDE; currently normalized as 'pending' | |
| return 'pending' | |
| } |
| content='Must pass independently to avoid automatic failure.' | ||
| triggerOn='hover' | ||
| > | ||
| <span className={styles.gatingMarker}>⚡</span> |
There was a problem hiding this comment.
The gating indicator is rendered as a raw "⚡" character inside a <span> with no accessible label. Screen readers may announce it ambiguously (or not at all). Please add an aria-label/title (or replace with an icon component that has accessible text) so gating is understandable for assistive tech users.
| <span className={styles.gatingMarker}>⚡</span> | |
| <span | |
| className={styles.gatingMarker} | |
| role='img' | |
| aria-label='Gating review: must pass independently to avoid automatic failure.' | |
| > | |
| ⚡ | |
| </span> |
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3907
What's in this PR?