PM-3907 refactor layout, move "rerun" tooltip to button,#1529
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const { mutate }: FullConfiguration = useSWRConfig() | ||
| const [rerunningRunId, setRerunningRunId] = useState<string | null>(null) | ||
|
|
||
| const handleRerun = useCallback(async (runId?: string): Promise<void> => { |
There was a problem hiding this comment.
[❗❗ 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.
| status={row.status} | ||
| score={row.score ?? undefined} | ||
| showScore={false} | ||
| action={row.run?.id && row.run?.id !== '-1' && isAdmin && row.status !== 'pending' && ( |
There was a problem hiding this comment.
[maintainability]
The condition row.run?.id && row.run?.id !== '-1' && isAdmin && row.status !== 'pending' is repeated multiple times. Consider extracting this logic into a descriptive function to improve maintainability and readability.
| hideLabel?: boolean | ||
| showScore?: boolean | ||
| submissionId?: string | ||
| action?: ReactNode |
There was a problem hiding this comment.
[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.
| label?: string | ||
| score?: number | ||
| status: 'pending' | 'failed' | 'passed' | 'failed-score' | ||
| action?: ReactNode |
There was a problem hiding this comment.
[❗❗ 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.
| </span> | ||
| )} | ||
| {!props.hideLabel && props.label} | ||
| {props.action} |
There was a problem hiding this comment.
[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.
| margin-left: -1 * $sp-4; | ||
| margin-right: -1 * $sp-4; | ||
| } | ||
| margin-top: -1px; |
There was a problem hiding this comment.
[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-top: -1px; | ||
| } | ||
| // margin-left: -1 * $sp-4; |
There was a problem hiding this comment.
[💡 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.
| if (parentTr && tbody) { | ||
| const createdTr = document.createElement('tr') | ||
| const createdTd = document.createElement('td') | ||
| const colCount = parentTd?.getAttribute('colSpan') ? parentTd.colSpan : parentTr.children.length || 1 |
There was a problem hiding this comment.
[💡 readability]
The logic for determining colCount could be improved for clarity. Consider using a more explicit conditional check for parentTd to avoid potential confusion with the fallback to parentTr.children.length.
|
|
||
| useEffect(() => { | ||
| // create portal row when opened | ||
| if (isOpen && wrapperRef.current) { |
There was a problem hiding this comment.
[correctness]
Ensure that wrapperRef.current is not null before accessing closest. Although this is checked, consider adding a more explicit null check or a guard clause to improve robustness.
| createdTr.parentElement.removeChild(createdTr) | ||
| createdRowRef.current = null | ||
| } | ||
| if (portalContainer) { |
There was a problem hiding this comment.
[💡 maintainability]
Resetting portalContainer to null in the cleanup function is redundant since it is already being set to null when the component unmounts. Consider removing this line to simplify the cleanup logic.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.tsx
Outdated
Show resolved
Hide resolved
src/apps/review/src/lib/components/CollapsibleAiReviewsRow/CollapsibleAiReviewsRow.tsx
Show resolved
Hide resolved
|
|
||
| const { isAdmin }: UseRolePermissionsResult = useRolePermissions() | ||
| const { mutate }: FullConfiguration = useSWRConfig() | ||
| const [rerunningRunId, setRerunningRunId] = useState<string | null>(null) |
There was a problem hiding this comment.
🟡 rerunningRunId state is tracked but never read, allowing concurrent re-run triggers
The rerunningRunId state (line 243) is set during handleRerun (lines 248, 257) but is never read anywhere — neither to disable the re-run button during a pending request nor to provide visual loading feedback. This is a regression from the old implementation in AiWorkflowRunStatus.tsx which used isRerunning to disable the button with disabled={isRerunning}. A user can now click the re-run icon multiple times, triggering concurrent retriggerAiWorkflowRun API calls for the same run.
Prompt for agents
In src/apps/review/src/lib/components/AiReviewsTable/AiReviewsTable.tsx, the rerunningRunId state (line 243) is set but never checked. Either:
1. Add a guard at the start of handleRerun: if (rerunningRunId) return;
2. Or pass rerunningRunId to the action button rendering logic (around lines 368 and 464) to conditionally disable or style the RefreshIcon, for example by checking rerunningRunId === row.run?.id.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| const { isAdmin }: UseRolePermissionsResult = useRolePermissions() | ||
| const { mutate }: FullConfiguration = useSWRConfig() | ||
| const [, setRerunningRunId] = useState<string | undefined>(undefined) |
There was a problem hiding this comment.
[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.
| handleError(error as Error) | ||
| toast.error('Failed to trigger workflow re-run.') | ||
| } finally { | ||
| setRerunningRunId(undefined) |
There was a problem hiding this comment.
[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.
| <Tooltip content='Re-run the workflow'> | ||
| <IconOutline.RefreshIcon | ||
| className={classNames('icon-lg', styles.reRunIcon)} | ||
| onClick={function onClick() { handleRerun(row.run!.id) }} |
There was a problem hiding this comment.
[💡 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 content='Re-run the workflow'> | ||
| <IconOutline.RefreshIcon | ||
| className={classNames('icon-lg', styles.reRunIcon)} | ||
| onClick={function onClick() { handleRerun(row.run!.id) }} |
There was a problem hiding this comment.
[💡 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.
| }, [aiReviewConfig, configuredWorkflows]) | ||
|
|
||
| const [isOpen, setIsOpen] = useState(props.defaultOpen ?? false) | ||
| const [portalContainer, setPortalContainer] = useState<HTMLTableCellElement | undefined>(undefined) |
There was a problem hiding this comment.
[correctness]
Switching from null to undefined for portalContainer may lead to unexpected behavior if any logic elsewhere in the codebase relies on strict null checks. Ensure that all usages of portalContainer are compatible with undefined values.
| const [isOpen, setIsOpen] = useState(props.defaultOpen ?? false) | ||
| const [portalContainer, setPortalContainer] = useState<HTMLTableCellElement | undefined>(undefined) | ||
| const wrapperRef = useRef<HTMLDivElement | null>(null) | ||
| const createdRowRef = useRef<HTMLTableRowElement | undefined>(undefined) |
There was a problem hiding this comment.
[correctness]
Changing createdRowRef from null to undefined could affect any logic that checks for null specifically. Verify that all references to createdRowRef handle undefined correctly.
| row.run?.id | ||
| && row.run?.id !== '-1' | ||
| && isAdmin | ||
| && row.status !== 'pending' | ||
| && ( |
There was a problem hiding this comment.
🟡 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).
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3907
What's in this PR?