fix: Exclude waiting_approval cards from active running state display#775
Conversation
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a visual discrepancy in the UI where cards awaiting approval were mistakenly presented with the styling and behavior of actively running tasks. By introducing a refined state variable, Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThe PR introduces an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the issue of cards with waiting_approval status being displayed as actively running. The introduction of the isActivelyRunning variable in both KanbanCard and ListRow components effectively encapsulates the new logic and fixes the UI inconsistency.
My main feedback, detailed in the comments, relates to maintainability. The new isActivelyRunning variable is passed to child components through props that are still named isCurrentAutoTask. This is potentially confusing as the prop's meaning has changed. Renaming these props in the child components would improve clarity.
Additionally, I noticed that AgentInfoPanel in kanban-card.tsx and IndicatorBadges in list-row.tsx are still receiving the original isCurrentAutoTask prop. This might be intentional, but it could lead to other subtle UI inconsistencies (e.g., error badges being hidden on waiting_approval cards). It would be beneficial to verify if these components should also be updated to use isActivelyRunning.
| ); | ||
| // 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'; |
There was a problem hiding this comment.
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 CardHeaderSection and CardActions via a prop that is still named isCurrentAutoTask. This can be misleading for future maintenance, as the prop's meaning has changed.
For better code clarity, I recommend renaming the isCurrentAutoTask prop to isActivelyRunning within the CardHeaderSection and CardActions components. Since those files are not part of this pull request, this could be addressed in a follow-up task.
| }: 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'; |
There was a problem hiding this comment.
This new variable correctly defines the active running state for list rows. However, it's then passed to the RowActions component via a prop named isCurrentAutoTask, which could be misleading for future developers.
To improve code clarity and maintainability, it would be best to rename the prop in the RowActions component to isActivelyRunning. This would make the component's API more explicit about its behavior.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/src/components/views/board-view/components/list-view/list-row.tsx (1)
244-244:⚠️ Potential issue | 🟡 MinorSame
hasErrorconcern as inkanban-card.tsx.This line still uses
isCurrentAutoTaskto suppress error styling. Awaiting_approvalcard that has an error and is still in the running tasks list will have its error hidden. If the kanban-card version is updated to useisActivelyRunning, this should be updated to match.Suggested change
- const hasError = feature.error && !isCurrentAutoTask; + const hasError = feature.error && !isActivelyRunning;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/list-view/list-row.tsx` at line 244, The hasError computation is suppressing error styling using isCurrentAutoTask which hides errors for waiting_approval cards; update the logic to match the kanban-card change by using isActivelyRunning instead of isCurrentAutoTask (i.e., compute hasError from feature.error && !isActivelyRunning) so running tasks still show error state; locate the hasError declaration in list-row.tsx and replace the isCurrentAutoTask reference with isActivelyRunning, ensuring any imports or upstream variable names are adjusted to match existing usage.apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx (1)
183-183:⚠️ Potential issue | 🟡 Minor
hasErrorshould useisActivelyRunningfor consistency with error styling onwaiting_approvalcardsThe current logic suppresses error styling for
waiting_approvalcards that remain in the running list, sincehasErrorgates on!isCurrentAutoTaskwhileisActivelyRunningexplicitly excludeswaiting_approvalstatus. SinceCardHeaderSectionandCardActionsalready useisActivelyRunningto handle the running state indicator,hasErrorshould do the same to ensure error styling displays consistently:Suggested change
- const hasError = feature.error && !isCurrentAutoTask; + const hasError = feature.error && !isActivelyRunning;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx` at line 183, The hasError computation currently uses feature.error && !isCurrentAutoTask which prevents error styling for cards like waiting_approval that are treated as running; change it to use isActivelyRunning (i.e., feature.error && !isActivelyRunning) so error styling is consistent with the running-state logic used by CardHeaderSection and CardActions; update the hasError definition to reference isActivelyRunning instead of isCurrentAutoTask.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx`:
- Around line 248-257: The prop name isCurrentAutoTask is semantically
mismatched because KanbanCard passes isActivelyRunning into it; rename the prop
to isActivelyRunning across the component boundary: update CardHeaderSection and
CardActions to accept a prop named isActivelyRunning (instead of
isCurrentAutoTask), update their prop types/prop interfaces, and update all
usages in KanbanCard (and the other occurrence where CardActions is rendered) to
pass isActivelyRunning, ensuring tests/types compile and any internal references
to isCurrentAutoTask are replaced with isActivelyRunning.
- Line 183: The hasError computation currently uses feature.error &&
!isCurrentAutoTask which prevents error styling for cards like waiting_approval
that are treated as running; change it to use isActivelyRunning (i.e.,
feature.error && !isActivelyRunning) so error styling is consistent with the
running-state logic used by CardHeaderSection and CardActions; update the
hasError definition to reference isActivelyRunning instead of isCurrentAutoTask.
In `@apps/ui/src/components/views/board-view/components/list-view/list-row.tsx`:
- Line 244: The hasError computation is suppressing error styling using
isCurrentAutoTask which hides errors for waiting_approval cards; update the
logic to match the kanban-card change by using isActivelyRunning instead of
isCurrentAutoTask (i.e., compute hasError from feature.error &&
!isActivelyRunning) so running tasks still show error state; locate the hasError
declaration in list-row.tsx and replace the isCurrentAutoTask reference with
isActivelyRunning, ensuring any imports or upstream variable names are adjusted
to match existing usage.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx`: - Around line 248-257: The prop name isCurrentAutoTask is semantically mismatched because KanbanCard passes isActivelyRunning into it; rename the prop to isActivelyRunning across the component boundary: update CardHeaderSection and CardActions to accept a prop named isActivelyRunning (instead of isCurrentAutoTask), update their prop types/prop interfaces, and update all usages in KanbanCard (and the other occurrence where CardActions is rendered) to pass isActivelyRunning, ensuring tests/types compile and any internal references to isCurrentAutoTask are replaced with isActivelyRunning.apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx (1)
248-257: PassingisActivelyRunningasisCurrentAutoTask— semantic mismatch in prop name.
CardHeaderSectionandCardActionsreceiveisActivelyRunningthrough a prop still namedisCurrentAutoTask. This works correctly for this fix, but the prop name no longer reflects what's being passed — it now means "actively running" rather than "is the current auto task." Consider renaming the prop in child components toisActivelyRunning(or similar) in a follow-up to avoid future confusion.Also applies to: 273-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx` around lines 248 - 257, The prop name isCurrentAutoTask is semantically mismatched because KanbanCard passes isActivelyRunning into it; rename the prop to isActivelyRunning across the component boundary: update CardHeaderSection and CardActions to accept a prop named isActivelyRunning (instead of isCurrentAutoTask), update their prop types/prop interfaces, and update all usages in KanbanCard (and the other occurrence where CardActions is rendered) to pass isActivelyRunning, ensuring tests/types compile and any internal references to isCurrentAutoTask are replaced with isActivelyRunning.
1 participant
Cards waiting for approval displayed as still running
Summary by CodeRabbit