Skip to content

PM-3907 refactor layout, move "rerun" tooltip to button, #1529

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
vas3a merged 3 commits into from
Mar 13, 2026
Merged

PM-3907 refactor layout, move "rerun" tooltip to button, #1529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15e3c42
PM-3907 #time 2h refactor layout, move "rerun" tooltip to button,
vas3a Mar 12, 2026
2bc2aff
lint
vas3a Mar 12, 2026
4ebc8e4
PM-3907 #time 15m pr feedback
vas3a Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
color: #C1294F;
border-radius: $sp-1;
padding: $sp-4;
margin-bottom: $sp-2;
margin: $sp-2 $sp-3;
Comment thread
vas3a marked this conversation as resolved.

display: flex;
gap: $sp-2;
Expand All @@ -32,6 +32,11 @@
white-space: normal;
}

.reRunIcon {
color: $black-80;
Comment thread
vas3a marked this conversation as resolved.
cursor: pointer;
}

.reviewsTable {
width: 100%;
border-collapse: collapse;
Expand Down
88 changes: 68 additions & 20 deletions src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import { FC, MouseEvent as ReactMouseEvent, useContext, useMemo } from 'react'
import { FC, MouseEvent as ReactMouseEvent, useCallback, useContext, useMemo, useState } from 'react'
import { Link } from 'react-router-dom'
import { toast } from 'react-toastify'
import { useSWRConfig } from 'swr'
import { FullConfiguration } from 'swr/dist/types'
import classNames from 'classnames'
import moment from 'moment'

import { handleError } from '~/libs/shared/lib/utils/handle-error'
import { useWindowSize, WindowSize } from '~/libs/shared'
import { IconOutline, Tooltip } from '~/libs/ui'

Expand All @@ -12,7 +16,11 @@ import {
AiWorkflowRun,
AiWorkflowRunsResponse,
AiWorkflowRunStatusEnum,
getAiWorkflowRunsCacheKey,
retriggerAiWorkflowRun,
useFetchAiWorkflowsRuns,
useRolePermissions,
UseRolePermissionsResult,
} from '../../hooks'
import { IconAiReview } from '../../assets/icons'
import { TABLE_DATE_FORMAT } from '../../../config/index.config'
Expand Down Expand Up @@ -170,7 +178,9 @@ const AiReviewsTable: FC<AiReviewsTableProps> = props => {
?? configured?.workflow?.scorecard?.minimumPassingScore

const status = fromDecision
? normalizeStatus(fromDecision.runStatus, fromDecision.runScore, minScore)
? normalizeStatus(run && aiRunInProgress(run)
? undefined
: fromDecision.runStatus, fromDecision.runScore, minScore)
: undefined

return {
Expand Down Expand Up @@ -230,6 +240,26 @@ const AiReviewsTable: FC<AiReviewsTableProps> = props => {

const loading = isLoading || isLoadingAiReviewConfig || isLoadingAiReviewDecisions

const { isAdmin }: UseRolePermissionsResult = useRolePermissions()
const { mutate }: FullConfiguration = useSWRConfig()
const [, setRerunningRunId] = useState<string | undefined>(undefined)
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

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

[⚠️ correctness]
Changing the state type from string | null to string | undefined for rerunningRunId might lead to unintended consequences if other parts of the codebase expect null as a valid state. Ensure that all usages of this state are compatible with undefined.


const handleRerun = useCallback(async (runId?: string): Promise<void> => {
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

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

[❗❗ correctness]
The handleRerun function uses run!.id, which assumes run is always defined when handleRerun is called. Consider adding a check to ensure run is not null or undefined before accessing run.id to prevent potential runtime errors.

if (!runId || runId === '-1') return

setRerunningRunId(runId)
try {
await retriggerAiWorkflowRun(runId)
await mutate(getAiWorkflowRunsCacheKey(props.submission.id))
toast.success('Workflow re-run triggered successfully.')
} catch (error) {
handleError(error as Error)
toast.error('Failed to trigger workflow re-run.')
} finally {
setRerunningRunId(undefined)
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

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

[⚠️ correctness]
The change from null to undefined for resetting rerunningRunId might affect logic that checks for null specifically. Verify that this change does not introduce any bugs related to state checks elsewhere in the application.

}
}, [mutate, props.submission.id])

const failedGatingReviewers = useMemo(
() => reviewerRows
.filter(row => row.isGating && (row.status === 'failed' || row.status === 'failed-score'))
Expand Down Expand Up @@ -334,15 +364,24 @@ const AiReviewsTable: FC<AiReviewsTableProps> = props => {
<div className={styles.mobileRow}>
<div className={styles.label}>Result</div>
<div className={`${styles.value} ${styles.resultCol}`}>
{row.status ? (
<AiWorkflowRunStatus
status={row.status}
score={row.score ?? undefined}
showScore={false}
/>
) : (
<AiWorkflowRunStatus run={row.run} submissionId={props.submission.id} />
)}
<AiWorkflowRunStatus
run={row.run}
status={row.status}
action={
row.run?.id
&& row.run?.id !== '-1'
&& isAdmin
&& row.status !== 'pending'
&& (
<Tooltip content='Re-run the workflow'>
<IconOutline.RefreshIcon
className={classNames('icon-lg', styles.reRunIcon)}
onClick={function onClick() { handleRerun(row.run!.id) }}
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

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

[💡 style]
The use of function onClick() { ... } syntax for the onClick handler is unconventional in JSX and might affect readability. Consider using an arrow function () => { ... } for consistency with common React practices.

/>
</Tooltip>
)
}
/>
</div>
</div>
</div>
Expand Down Expand Up @@ -427,15 +466,24 @@ const AiReviewsTable: FC<AiReviewsTableProps> = props => {
) : '-'}
</td>
<td className={styles.resultCol}>
{row.status ? (
<AiWorkflowRunStatus
status={row.status}
score={row.score ?? undefined}
showScore={false}
/>
) : (
<AiWorkflowRunStatus run={row.run} submissionId={props.submission.id} />
)}
<AiWorkflowRunStatus
status={row.status}
run={row.run}
action={
row.run?.id
&& row.run?.id !== '-1'
&& isAdmin
&& row.status !== 'pending'
&& (
Comment on lines +473 to +477
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot Mar 12, 2026

Choose a reason for hiding this comment

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

🟡 Re-run button shown for in-progress runs when row.status is undefined

When fromDecision is undefined but a run exists (e.g., a newly triggered workflow with no decision yet), row.status is set to undefined (AiReviewsTable.tsx:184). The action visibility check row.status !== 'pending' at lines 476 and 374 evaluates to true (since undefined !== 'pending'), so the re-run icon is rendered. However, inside AiWorkflowRunStatus, the computed displayStatus may resolve to 'pending' via aiRunStatus(props.run) when the run is in progress. The old code checked the computed displayStatus === 'pending' inside the Wrapper component to suppress the re-run button, but the new code checks only row.status which doesn't cover this case. This allows admins to trigger re-runs on already in-progress workflows.

Prompt for agents 
In src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.tsx, the action visibility check at lines 370-374 (tablet view) and 473-476 (desktop view) uses `row.status !== 'pending'` to decide whether to show the re-run icon. However, when `row.status` is undefined (no decision data exists), this check passes even if the run is actually in-progress. The fix should also account for the run's own status. Change the condition to something like:

  row.run?.id
  && row.run?.id !== '-1'
  && isAdmin
  && row.status !== 'pending'
  && !(row.run && aiRunInProgress(row.run))

Apply this fix in both the tablet view (around line 370-374) and the desktop view (around line 473-476).
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

<Tooltip content='Re-run the workflow'>
<IconOutline.RefreshIcon
className={classNames('icon-lg', styles.reRunIcon)}
onClick={function onClick() { handleRerun(row.run!.id) }}
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

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

[💡 style]
The use of function onClick() { ... } syntax for the onClick handler is unconventional in JSX and might affect readability. Consider using an arrow function () => { ... } for consistency with common React practices.

/>
</Tooltip>
)
}
/>
</td>
</tr>
))}
Expand Down
87 changes: 10 additions & 77 deletions src/apps/review/src/lib/components/AiReviewsTable/AiWorkflowRunStatus.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,18 @@
import { FC, PropsWithChildren, useCallback, useMemo, useState } from 'react'
import { toast } from 'react-toastify'
import { useSWRConfig } from 'swr'
import { FullConfiguration } from 'swr/dist/types'
import { FC, ReactNode, useMemo } from 'react'

import { IconOutline, Tooltip } from '~/libs/ui'
import { handleError } from '~/libs/shared/lib/utils/handle-error'
import { IconOutline } from '~/libs/ui'

import {
aiRunFailed,
aiRunInProgress,
AiWorkflowRun,
getAiWorkflowRunsCacheKey,
retriggerAiWorkflowRun,
useRolePermissions,
UseRolePermissionsResult,
} from '../../hooks'
import { aiRunFailed, aiRunInProgress, AiWorkflowRun } from '../../hooks'

import StatusLabel from './StatusLabel'
import styles from './AiWorkflowRunStatus.module.scss'

interface AiWorkflowRunStatusProps {
run?: Pick<AiWorkflowRun, 'status'|'score'|'workflow'|'id'>
status?: 'passed' | 'pending' | 'failed-score' | 'failed'
score?: number
hideLabel?: boolean
showScore?: boolean
submissionId?: string
action?: ReactNode
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The action prop is introduced but not used in the current component logic. Ensure that it is necessary and used appropriately, or consider removing it if it's not needed.

}

const aiRunStatus = (run: Pick<AiWorkflowRun, 'status'|'score'|'workflow'>): string => {
Expand All @@ -41,10 +28,6 @@ const aiRunStatus = (run: Pick<AiWorkflowRun, 'status'|'score'|'workflow'>): str
}

export const AiWorkflowRunStatus: FC<AiWorkflowRunStatusProps> = props => {
const [isRerunning, setIsRerunning] = useState(false)
const { isAdmin }: UseRolePermissionsResult = useRolePermissions()
const { mutate }: FullConfiguration = useSWRConfig()

const status = useMemo(() => {
if (props.status) {
return props.status
Expand All @@ -58,71 +41,18 @@ export const AiWorkflowRunStatus: FC<AiWorkflowRunStatusProps> = props => {
}, [props.status, props.run])

const displayStatus = status

const handleRerun = useCallback(async (): Promise<void> => {
const runId = props.run?.id
if (!runId || runId === '-1') {
return
}

setIsRerunning(true)

try {
await retriggerAiWorkflowRun(runId)

if (props.submissionId) {
await mutate(getAiWorkflowRunsCacheKey(props.submissionId))
} else {
await mutate(
(key: unknown) => typeof key === 'string' && key.includes('/workflows/runs?submissionId='),
)
}

toast.success('Workflow re-run triggered successfully.')
} catch (error) {
handleError(error as Error)
toast.error('Failed to trigger workflow re-run.')
} finally {
setIsRerunning(false)
}
}, [mutate, props.run, props.submissionId])

const score: number | undefined = props.showScore ? (props.score ?? props.run?.score) : undefined

const Wrapper: FC<PropsWithChildren> = useCallback(({ children }: PropsWithChildren) => {
if (!isAdmin || displayStatus === 'pending' || !props.run?.id || props.run?.id === '-1') {
return <>{children}</>
}

return (
<Tooltip
clickable
content={(
<button
type='button'
className={styles.reRunButton}
disabled={isRerunning}
onClick={handleRerun}
>
<IconOutline.ArrowRightIcon className='icon-sm' style={{ marginRight: '0.5rem' }} />
{isRerunning ? 'Re-running...' : 'Re-run'}
</button>
)}
>
{children}
</Tooltip>
)
}, [isAdmin, displayStatus, props.run, isRerunning, handleRerun])

return (
<Wrapper>
<>
{displayStatus === 'passed' && (
<StatusLabel
icon={<IconOutline.CheckIcon className='icon-xl' />}
hideLabel={props.hideLabel}
label='Passed'
status={displayStatus}
score={score}
action={props.action}
/>
)}
{displayStatus === 'failed-score' && (
Expand All @@ -132,6 +62,7 @@ export const AiWorkflowRunStatus: FC<AiWorkflowRunStatusProps> = props => {
label='Failed'
status={displayStatus}
score={score}
action={props.action}
/>
)}
{displayStatus === 'pending' && (
Expand All @@ -141,6 +72,7 @@ export const AiWorkflowRunStatus: FC<AiWorkflowRunStatusProps> = props => {
label='To be filled'
status={displayStatus}
score={score}
action={props.action}
/>
)}
{displayStatus === 'failed' && (
Expand All @@ -150,8 +82,9 @@ export const AiWorkflowRunStatus: FC<AiWorkflowRunStatusProps> = props => {
status={displayStatus}
label='Failure'
score={score}
action={props.action}
/>
)}
</Wrapper>
</>
)
}
2 changes: 2 additions & 0 deletions src/apps/review/src/lib/components/AiReviewsTable/StatusLabel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ interface StatusLabelProps {
label?: string
score?: number
status: 'pending' | 'failed' | 'passed' | 'failed-score'
action?: ReactNode
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

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

[❗❗ security]
The action prop is added as an optional ReactNode. Ensure that any components passed as action are properly sanitized and do not introduce security vulnerabilities, especially if they involve user-generated content.

}

const StatusLabel: FC<StatusLabelProps> = props => (
Expand All @@ -22,6 +23,7 @@ const StatusLabel: FC<StatusLabelProps> = props => (
</span>
)}
{!props.hideLabel && props.label}
{props.action}
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

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

[⚠️ design]
Rendering props.action directly could lead to unexpected layout shifts or styling issues if the content is not controlled. Consider adding a wrapper or style constraints to ensure consistent layout.

</div>
)

Expand Down
15 changes: 8 additions & 7 deletions ...pps/review/src/lib/components/CollapsibleAiReviewsRow/CollapsibleAiReviewsRow.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,15 @@
}

.table {
margin-top: $sp-2;
margin-left: -1 * $sp-4;
@include ltelg {
margin-top: 0;
margin-left: -1 * $sp-4;
margin-right: -1 * $sp-4;
}
margin-top: -1px;
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

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

[⚠️ correctness]
The change from margin-top: $sp-2; to margin-top: -1px; could lead to layout issues, especially if $sp-2 was a significant positive value. Ensure that this change does not negatively impact the layout across different screen sizes.

}
// margin-left: -1 * $sp-4;
Copy link
Copy Markdown

@github-actions github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

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

[💡 maintainability]
The commented-out code for margin-left and the @include ltelg block suggests that these styles might still be relevant. If these styles are no longer needed, consider removing them entirely to avoid confusion. If they might be needed later, document the reason for commenting them out elsewhere.

// @include ltelg {
// margin-top: 0;
// margin-left: -1 * $sp-4;
// margin-right: -1 * $sp-4;
// }
// }

.rotated {
transform: rotate(180deg);
Expand Down
Loading
Loading