Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

PM-1905 - ai reviews#1277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
kkartunov merged 5 commits intofeat/ai-workflowsfromPM-1905_review-details-page
Nov 3, 2025

Conversation

@vas3a
Copy link
Collaborator

Related JIRA Ticket:

https://topcoder.atlassian.net/browse/PM-1905

What's in this PR?

imageimage

Note: reviews details will be fetched only on row expand:
image

}

.workflowName {
max-width:200px;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[💡design]
The.workflowName class usesmax-width: 200px withwhite-space: nowrap,overflow: hidden, andtext-overflow: ellipsis. This combination will truncate text that exceeds 200px, which might not be suitable for all content. Consider whether this truncation is acceptable for all cases.

return(
<divclassName={styles.wrap}>
<tableclassName={styles.reviewsTable}>
<tr>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[⚠️readability]
The<tr> elements should be wrapped in a<thead> and<tbody> for better semantic HTML structure and accessibility.

<>
<MinusCircleIconclassName='icon icon-xl'/>
{' '}
Passed

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[❗❗correctness]
The text 'Passed' is used in both branches of the conditional rendering. It seems like a mistake since the icon suggests a different result. Consider changing the text in the else branch to 'Failed' or another appropriate term.

if(SUBMISSION_TAB_KEYS.has(selectedTabNormalized)){
returnrenderSubmissionTab({
aiReviewers:(
challengeInfo?.reviewers?.filter(r=>!!r.aiWorkflowId)as{aiWorkflowId:string}[]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[💡maintainability]
The use offilter(r => !!r.aiWorkflowId) assumes thataiWorkflowId is the only property needed fromreviewers. If additional properties are required in the future, this filtering logic might need to be updated. Consider whether this assumption is safe for future changes.

},
type:'element',
},
...(!props.aiReviewers?.length ?[] :[{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[⚠️correctness]
ThepropertyName for the 'Reviewer' column is set to 'submittedDate', which seems incorrect. This could lead to confusion or errors when accessing the column data. Consider using a more appropriate property name that reflects the column's purpose.

propertyName:'submittedDate',
renderer:(submission:BackendSubmission)=>(
<CollapsibleAiReviewsRow
aiReviewers={props.aiReviewers!}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[❗❗correctness]
Using the non-null assertion operator! onprops.aiReviewers assumes that it is always defined when this code executes. Ifprops.aiReviewers can be undefined, this could lead to runtime errors. Consider adding a check or default value to ensure safety.

text-align:left;
}

.reviewersDropown {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[💡readability]
The class name.reviewersDropown appears to have a typo. It should likely be.reviewersDropdown for clarity and consistency.


.table {
margin-top:$sp-2;
margin-left:-1*$sp-4;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[💡readability]
Using-1 * $sp-4 formargin-left might be less clear than simply using-$sp-4. Consider simplifying the expression for better readability.


return(
<divclassName={styles.wrap}>
<spanclassName={styles.reviewersDropown}onClick={toggleOpen}>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[⚠️accessibility]
Consider addingrole='button' andtabIndex='0' to the<span> to improve accessibility for keyboard users and screen readers.

{' '}
AI Reviewer
{aiReviewersCount===1 ?'' :'s'}
<IconOutline.ChevronDownIconclassName='icon-xl'/>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[💡usability]
Ensure that theChevronDownIcon rotates when the dropdown is open to provide a visual cue to users.

}
}

.table.aiReviewersTableRow.aiReviewersTableRow {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[⚠️maintainability]
The selector.table .aiReviewersTableRow.aiReviewersTableRow seems to have a duplicated class name. This could lead to confusion or unintended styling issues. Consider removing the duplicate class name.

[props.submissions],
)

const[toggledRows,setToggledRows]=useState(newSet<string>())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[❗❗correctness]
UsinguseState with aSet can lead to unexpected behavior because React does not detect changes to the internal state of aSet. Consider using an array or another data structure that React can track changes to more effectively.

<span>-</span>
)}
</td>
{!!props.aiReviewers?.length&&(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[⚠️design]
The conditional rendering of the AI Reviewer column header is based on the presence ofprops.aiReviewers. Ifprops.aiReviewers is undefined or an empty array, this column will not be rendered, which might lead to inconsistent table structures. Ensure that the table structure remains consistent regardless of the data.

</td>
</tr>
</tr>
{toggledRows.has(submission.id)&&(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[⚠️maintainability]
ThecolSpan value of 4 assumes a fixed number of columns. If the table structure changes, this could lead to layout issues. Consider dynamically calculatingcolSpan based on the number of columns to ensure future maintainability.

error:fetchError,
isValidating:isLoading,
}:SWRResponse<AiWorkflowRun[],Error>=useSWR<AiWorkflowRun[],Error>(
`${TC_API_BASE_URL}/workflows/${workflowIds.join(',')}/runs?submissionId=${submissionId}`,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[⚠️performance]
TheuseSWR hook is using a customfetcher function that makes multiple requests usingPromise.all. This approach could lead to performance issues ifworkflowIds is large, as it will send all requests concurrently. Consider implementing a batching mechanism or limiting the number of concurrent requests to improve performance.

}
}

exportfunctionuseFetchAiWorkflowsRuns(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[💡maintainability]
The function nameuseFetchAiWorkflowsRuns is very similar touseFetchAiWorkflowRuns, which could lead to confusion. Consider renaming one of the functions to more clearly distinguish their purposes.

incrementalPayment:number
type:string
isAIReviewer:boolean
aiWorkflowId?:string;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[❗❗correctness]
ChangingisAIReviewer from aboolean toaiWorkflowId as anoptional string alters the data model significantly. Ensure that all parts of the codebase that rely on this field are updated accordingly to handle the new type and potentialundefined value. This change could impact logic that previously depended on a boolean check.

Copy link
Collaborator

@kkartunovkkartunov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good

@kkartunovkkartunov merged commit5060ce1 intofeat/ai-workflowsNov 3, 2025
5 checks passed
@vas3avas3a deleted the PM-1905_review-details-page branchNovember 4, 2025 10:34
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@kkartunovkkartunovkkartunov approved these changes

@jmgasperjmgasperAwaiting requested review from jmgasper

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vas3a@kkartunov

[8]ページ先頭

©2009-2025 Movatter.jp