Conversation
…figuration PM-3851 ai review configuration
…t be editable in the UI
…nto PM-3813_ai-reviewer-configs
…nto PM-3813_ai-reviewer-configs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
.circleci/config.yml
Outdated
| context: org-global | ||
| filters: &filters-dev | ||
| branches: | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
[❗❗ correctness]
There is a merge conflict marker in the code. This needs to be resolved before merging. Ensure that the correct branches are specified in the only filter.
| } | ||
|
|
||
| return scorecards.filter(scorecard => ( | ||
| scorecard && |
There was a problem hiding this comment.
[💡 style]
The check scorecard && is redundant here because scorecards is already defaulted to an empty array, and filter will skip undefined elements.
| incrementalCoefficient: defaultReviewer.incrementalCoefficient | ||
| }) | ||
|
|
||
| if (updatedReviewers[index] && (updatedReviewers[index].isMemberReview !== false)) { |
There was a problem hiding this comment.
[correctness]
The condition updatedReviewers[index] && (updatedReviewers[index].isMemberReview !== false) is potentially problematic if updatedReviewers[index] is undefined. Consider ensuring updatedReviewers[index] is always defined before this point or handle the case where it might be undefined.
| )) | ||
| const fallbackScorecardId = hasDefaultScorecard | ||
| ? defaultScorecardId | ||
| : normalizeIdValue(scorecardsForPhase[0] && scorecardsForPhase[0].id) |
There was a problem hiding this comment.
[correctness]
The fallback logic for fallbackScorecardId assumes that scorecardsForPhase always has at least one element. Consider adding a check to ensure scorecardsForPhase is not empty before accessing scorecardsForPhase[0].id.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
src/components/ChallengeEditor/ChallengeReviewer-Field/HumanReviewTab.js
Outdated
Show resolved
Hide resolved
| const assignedPhaseIds = new Set( | ||
| (challenge.reviewers || []) | ||
| .filter((r, i) => i !== index && (r.isMemberReview !== false)) | ||
| .map(r => r.phaseId) | ||
| .filter(id => id !== undefined && id !== null) | ||
| ) |
There was a problem hiding this comment.
🔴 HumanReviewTab phase filter excludes wrong reviewer due to index mismatch with full reviewers array
In renderReviewerForm, the assignedPhaseIds filter at line 472-473 iterates over challenge.reviewers (the full array including AI reviewers) and excludes items where i !== index. But index is the human-reviewer-filtered index, while i is the index in the full challenge.reviewers array. When AI reviewers are present, this excludes the wrong reviewer from the phase-assignment check, potentially hiding valid phase options or showing phases that should be excluded.
Prompt for agents
In src/components/ChallengeEditor/ChallengeReviewer-Field/HumanReviewTab.js, in renderReviewerForm (around line 471-476), the assignedPhaseIds computation filters challenge.reviewers using i !== index, but index is the human-filtered index while i iterates over the full reviewers array (including AI reviewers). You need to compute the actual index of the current reviewer in the full challenge.reviewers array first (similar to how updateReviewer does it with indexOf), then use that actualIndex for the exclusion filter. Change the filter from (r, i) => i !== index to exclude the actual reviewer object, for example by comparing against the reviewer object reference or using the actual index in the full array.
Was this helpful? React with 👍 or 👎 to provide feedback.
src/components/ChallengeEditor/ChallengeReviewer-Field/ReviewSummary/ReviewSummary.js
Outdated
Show resolved
Hide resolved
.../ChallengeEditor/ChallengeReviewer-Field/AiReviewerTab/components/AiWorkflowsTableListing.js
Outdated
Show resolved
Hide resolved
src/components/ChallengeEditor/ChallengeReviewer-Field/ReviewSummary/ReviewSummary.js
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/components/ChallengeEditor/ChallengeReviewer-Field/HumanReviewTab.js
Outdated
Show resolved
Hide resolved
| const assignedPhaseIds = new Set( | ||
| (challenge.reviewers || []) | ||
| .filter((r, i) => i !== index && (r.isMemberReview !== false)) | ||
| .map(r => r.phaseId) | ||
| .filter(id => id !== undefined && id !== null) | ||
| ) |
src/components/ChallengeEditor/ChallengeReviewer-Field/ReviewSummary/ReviewSummary.js
Outdated
Show resolved
Hide resolved
.../ChallengeEditor/ChallengeReviewer-Field/AiReviewerTab/components/AiWorkflowsTableListing.js
Outdated
Show resolved
Hide resolved
src/components/ChallengeEditor/ChallengeReviewer-Field/ReviewSummary/ReviewSummary.js
Outdated
Show resolved
Hide resolved
src/components/ChallengeEditor/ChallengeReviewer-Field/AiReviewerTab/AiReviewTab.js
Show resolved
Hide resolved
| const updatedReviewers = allChallengeReviewers.filter((_, i) => i !== actualIndex) | ||
| onUpdateReviewers({ field: 'reviewers', value: updatedReviewers }) | ||
| } | ||
| }, [challenge.reviewers, onUpdateReviewers, aiReviewers]) |
There was a problem hiding this comment.
[performance]
The addition of aiReviewers to the dependency array of the useCallback hook is correct to ensure that the callback updates when aiReviewers changes. However, ensure that aiReviewers is stable and doesn't change unnecessarily, as it could cause unnecessary re-renders or function re-creations.
| <tbody> | ||
| {workflows.map((workflow, index) => { | ||
| const isAssigned = (challenge.reviewers || []).some(r => r.aiWorkflowId === workflow.workflowId) | ||
| const workflowDetails = (availableWorkflows || []).find(w => w.id === workflow.workflowId) || {} |
There was a problem hiding this comment.
[💡 maintainability]
The use of (availableWorkflows || []) is a defensive programming practice to handle cases where availableWorkflows might be null or undefined. However, since availableWorkflows has a default value of an empty array in defaultProps, this check might be redundant. Consider removing it to simplify the code.
| const newAssigned = currentAssigned.slice(0, newCount) | ||
|
|
||
| for (const member of removedMembers) { | ||
| if (member && member.roleId && member.handle) { |
There was a problem hiding this comment.
[❗❗ correctness]
The deleteResource function now requires three parameters: challenge.id, member.roleId, and member.handle. Ensure that the deleteResource function is updated accordingly to handle these parameters correctly.
| incrementalCoefficient: defaultReviewer.incrementalCoefficient | ||
| }) | ||
|
|
||
| if (updatedReviewers[actualIndex] && (updatedReviewers[actualIndex].isMemberReview !== false)) { |
There was a problem hiding this comment.
[❗❗ correctness]
The index variable index was replaced with actualIndex. Ensure that actualIndex is correctly calculated and corresponds to the intended reviewer in the updatedReviewers array.
| const firstPlacePrize = this.getFirstPlacePrizeValue(challenge) | ||
| const estimatedSubmissionsCount = 2 | ||
|
|
||
| const reviewersCost = calculateReviewCost(reviewers, challenge) |
There was a problem hiding this comment.
[❗❗ correctness]
The calculation of reviewersCost has been refactored to use calculateReviewCost. Ensure that this function is thoroughly tested and handles all edge cases, as it is critical for calculating costs accurately.
| const role = getResourceRoleByName(metadata.resourceRoles || [], roleName) | ||
| const resourceRoleId = role && role.id | ||
|
|
||
| if (!resourceRoleId) return '' |
There was a problem hiding this comment.
[correctness]
Returning an empty string instead of an empty array may lead to unexpected behavior if the caller expects an array. Consider returning an empty array to maintain consistency with the expected return type.
|
|
||
| const totalHumanReviewerCount = humanReviewers.reduce((sum, r) => sum + (parseInt(r.memberReviewerCount) || 1), 0) | ||
|
|
||
| const reviewCost = calculateReviewCost(humanReviewers, challenge) |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that the calculateReviewCost function handles all edge cases, such as when humanReviewers or challenge is undefined or null. This will prevent potential runtime errors.
| export function calculateReviewCost(humanReviewers = [], challenge = {}) { | ||
| const total = humanReviewers | ||
| .reduce((sum, r) => { | ||
| const memberCount = parseInt(r.memberReviewerCount) || 1 |
There was a problem hiding this comment.
[correctness]
Using parseInt without a radix can lead to unexpected results if the input string starts with '0'. Consider specifying a radix, e.g., parseInt(r.memberReviewerCount, 10).
| const total = humanReviewers | ||
| .reduce((sum, r) => { | ||
| const memberCount = parseInt(r.memberReviewerCount) || 1 | ||
| const baseAmount = parseFloat(r.fixedAmount) || 0 |
There was a problem hiding this comment.
[correctness]
Using parseFloat on potentially undefined values like r.fixedAmount could lead to NaN if r.fixedAmount is not a valid number. Consider validating the input before parsing.
| const memberCount = parseInt(r.memberReviewerCount) || 1 | ||
| const baseAmount = parseFloat(r.fixedAmount) || 0 | ||
| const prizeSet = challenge.prizeSets && challenge.prizeSets[0] | ||
| const prizeValue = prizeSet && prizeSet.prizes && prizeSet.prizes[0] && prizeSet.prizes[0].value |
There was a problem hiding this comment.
[❗❗ correctness]
The nested access of prizeSet.prizes[0].value assumes the structure is always present. Consider adding checks to ensure prizes is an array and has at least one element to avoid runtime errors.
| : 0 | ||
|
|
||
| const estimatedSubmissions = 2 | ||
| const baseCoefficient = parseFloat(r.baseCoefficient) || 0.13 |
There was a problem hiding this comment.
[correctness]
Using parseFloat on potentially undefined values like r.baseCoefficient could lead to NaN if r.baseCoefficient is not a valid number. Consider validating the input before parsing.
|
|
||
| const estimatedSubmissions = 2 | ||
| const baseCoefficient = parseFloat(r.baseCoefficient) || 0.13 | ||
| const incrementalCoefficient = parseFloat(r.incrementalCoefficient) || 0.05 |
There was a problem hiding this comment.
[correctness]
Using parseFloat on potentially undefined values like r.incrementalCoefficient could lead to NaN if r.incrementalCoefficient is not a valid number. Consider validating the input before parsing.
| return sum + calculatedCost | ||
| }, 0) | ||
|
|
||
| return total.toFixed(2) |
There was a problem hiding this comment.
[💡 design]
Returning a string from toFixed might not be suitable if the caller expects a number. Consider returning a number by using parseFloat(total.toFixed(2)).
| export function calculateReviewCost (humanReviewers = [], challenge = {}) { | ||
| const total = humanReviewers | ||
| .reduce((sum, r) => { | ||
| const memberCount = parseInt(r.memberReviewerCount) || 1 |
There was a problem hiding this comment.
[❗❗ correctness]
Using parseInt without a radix can lead to unexpected results if the input string starts with '0'. Consider specifying a radix, e.g., parseInt(r.memberReviewerCount, 10).
| const total = humanReviewers | ||
| .reduce((sum, r) => { | ||
| const memberCount = parseInt(r.memberReviewerCount) || 1 | ||
| const baseAmount = parseFloat(r.fixedAmount) || 0 |
There was a problem hiding this comment.
[correctness]
Using parseFloat without validation can lead to NaN if the input is not a valid number. Consider adding validation to ensure r.fixedAmount is a valid number before parsing.
| : 0 | ||
|
|
||
| const estimatedSubmissions = 2 | ||
| const baseCoefficient = parseFloat(r.baseCoefficient) || 0.13 |
There was a problem hiding this comment.
[correctness]
Using parseFloat without validation can lead to NaN if the input is not a valid number. Consider adding validation to ensure r.baseCoefficient is a valid number before parsing.
|
|
||
| const estimatedSubmissions = 2 | ||
| const baseCoefficient = parseFloat(r.baseCoefficient) || 0.13 | ||
| const incrementalCoefficient = parseFloat(r.incrementalCoefficient) || 0.05 |
There was a problem hiding this comment.
[correctness]
Using parseFloat without validation can lead to NaN if the input is not a valid number. Consider adding validation to ensure r.incrementalCoefficient is a valid number before parsing.
|
@kkartunov I addressed the concerns 👍 |
| const resetConfiguration = useCallback((newConfig = initialConfig) => { | ||
| setConfiguration(newConfig) | ||
| }, [setConfiguration]) |
There was a problem hiding this comment.
🟡 resetConfiguration captures stale initialConfig closure
In useConfigurationState.js at line 85, resetConfiguration uses initialConfig as a default parameter value, but initialConfig is captured in the closure from the hook's first call. Since initialConfig is a default parameter with an object literal, this is stable. However, the useCallback dependency array only lists [setConfiguration] and omits initialConfig. If the caller ever passes a different initialConfig on re-render, resetConfiguration() would use the stale initial value. In practice this is unlikely since the default is always used, making this non-severe.
Was this helpful? React with 👍 or 👎 to provide feedback.
| challenge.track.name, | ||
| challenge.type.name |
There was a problem hiding this comment.
🔴 useTemplateManager crashes with challengeTrack.toUpperCase() when challengeTrack is undefined
In useTemplateManager.js at line 50, challengeTrack.toUpperCase() is called without null-checking. The loadTemplates callback is invoked from the effect at line 63-67 only when challengeTrack && challengeType are truthy, but loadTemplates itself is a standalone callback that could be called from outside. More critically, TemplateConfigurationView.js:33 passes challenge.track.name and challenge.type.name — if challenge.track or challenge.type is a string rather than an object (which is possible given the codebase handles both cases at src/components/ChallengeEditor/ChallengeReviewer-Field/index.js:53-58), accessing .name on a string returns undefined, and .toUpperCase() on undefined throws.
| challenge.track.name, | |
| challenge.type.name | |
| typeof challenge.track === 'string' ? challenge.track : challenge.track.name, | |
| typeof challenge.type === 'string' ? challenge.type : challenge.type.name |
Was this helpful? React with 👍 or 👎 to provide feedback.
This was never merged into dev. we tested & QAd it via feature branch.