-
Notifications
You must be signed in to change notification settings - Fork 577
fix: Exclude waiting_approval cards from active running state display #775
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -209,6 +209,10 @@ export const ListRow = memo(function ListRow({ | |
| blockingDependencies = [], | ||
| className, | ||
| }: ListRowProps) { | ||
| // A card in waiting_approval should not display as "actively running" even if | ||
| // it's still in the runningAutoTasks list. The waiting_approval UI takes precedence. | ||
| const isActivelyRunning = isCurrentAutoTask && feature.status !== 'waiting_approval'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new variable correctly defines the active running state for list rows. However, it's then passed to the To improve code clarity and maintainability, it would be best to rename the prop in the |
||
|
|
||
| const handleRowClick = useCallback( | ||
| (e: React.MouseEvent) => { | ||
| // Don't trigger row click if clicking on checkbox or actions | ||
|
|
@@ -349,13 +353,13 @@ export const ListRow = memo(function ListRow({ | |
|
|
||
| {/* Actions column */} | ||
| <div role="cell" className="flex items-center justify-end px-3 py-3 w-[80px] shrink-0"> | ||
| <RowActions feature={feature} handlers={handlers} isCurrentAutoTask={isCurrentAutoTask} /> | ||
| <RowActions feature={feature} handlers={handlers} isCurrentAutoTask={isActivelyRunning} /> | ||
| </div> | ||
| </div> | ||
| ); | ||
|
|
||
| // Wrap with animated border for currently running auto task | ||
| if (isCurrentAutoTask) { | ||
| if (isActivelyRunning) { | ||
| return <div className="animated-border-wrapper-row">{rowContent}</div>; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new variable correctly encapsulates the logic for when a card should be displayed as 'actively running'. However, this variable is then passed to child components like
CardHeaderSectionandCardActionsvia a prop that is still namedisCurrentAutoTask. This can be misleading for future maintenance, as the prop's meaning has changed.For better code clarity, I recommend renaming the
isCurrentAutoTaskprop toisActivelyRunningwithin theCardHeaderSectionandCardActionscomponents. Since those files are not part of this pull request, this could be addressed in a follow-up task.