Refactor kanban cards #194
Conversation
…and functionality - Moved KanbanCard logic into separate files for better modularity, including card actions, badges, content sections, and agent info panel. - Updated import paths to reflect new file structure. - Enhanced readability and maintainability of the KanbanCard component by breaking it down into smaller, focused components. - Removed the old KanbanCard implementation and replaced it with a new, organized structure that supports better code management.
…onents - Removed the onCommit action from KanbanBoard and related components to streamline functionality. - Updated CardActions to replace the Commit button with a Mark as Verified button, enhancing clarity in user interactions. - Introduced a new CardBadge component for consistent styling of badges across KanbanCard, improving code reusability and maintainability. - Refactored badge rendering logic to include a Just Finished badge, ensuring accurate representation of feature status.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe monolithic KanbanCard component was removed and replaced by a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (2)
Comment |
Summary of ChangesHello @webdevcody, 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 significantly refactors the 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. 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
|
![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 is a great refactoring of the massive KanbanCard component into smaller, more manageable sub-components. This significantly improves the readability and maintainability of the code.
I've noticed one functional change alongside the refactoring: the 'Commit' button on cards in the 'Waiting Approval' column has been replaced with a 'Mark as Verified' button. This seems intentional as the onCommit prop has been removed. It would be good to mention this in the pull request description if it's not already covered elsewhere.
I have a few minor suggestions to further improve the code quality.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any, no-undef | ||
| const currentProject = (window as any).__currentProject; |
There was a problem hiding this comment.
I've noticed several eslint-disable-next-line no-undef comments for standard browser globals like window, console, setInterval, and clearInterval in this file and card-badges.tsx. This suggests a potential misconfiguration in your ESLint setup. Instead of disabling the rule, it would be better to configure the ESLint environment to recognize browser globals. You can typically do this by adding env: { browser: true } to your ESLint configuration file (e.g., .eslintrc.js). This will make the code cleaner and ensure the linter works as expected for a web project.
| // eslint-disable-next-line no-undef | ||
| const interval = setInterval(() => { | ||
| setCurrentTime(Date.now()); | ||
| }, 1000); |
There was a problem hiding this comment.
Similar to agent-info-panel.tsx, there are eslint-disable-next-line no-undef comments here for setInterval and clearInterval. Please see my other comment about configuring the ESLint environment to avoid disabling this rule for standard browser globals.
| onResume, | ||
| onForceStop, | ||
| onManualVerify, | ||
| onMoveBackToInProgress: _onMoveBackToInProgress, |
There was a problem hiding this comment.
The onMoveBackToInProgress prop is destructured with an underscore, indicating it's unused within this component. It seems this prop is passed down through KanbanBoard but isn't actually used by any of the card's actions. This might be an opportunity to remove this prop entirely to clean up the component's API, unless it's planned for future use.
Code Review: Refactor Kanban CardsSummaryThis is an excellent refactoring that breaks down a monolithic 1332-line component into smaller, focused modules. The changes significantly improve maintainability and code organization. Overall, this is a well-executed refactor that follows clean code principles. ✅ Strengths1. Excellent Component Decomposition
2. Improved Maintainability
3. Preserved Functionality
4. Good Abstraction Choices
🔍 Issues & Recommendations1. Unused Prop - Missing onCommit Callback
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx (4)
51-58: Consider simplifying the passthrough function.
handleConfirmDeleteis a passthrough that adds no logic. While it's fine for future extensibility, you could passonDeletedirectly toDeleteConfirmDialog.🔎 Suggested simplification
- const handleConfirmDelete = () => { - onDelete(); - }; ... <DeleteConfirmDialog open={isDeleteDialogOpen} onOpenChange={setIsDeleteDialogOpen} - onConfirm={handleConfirmDelete} + onConfirm={onDelete}
129-190: Redundant Fragment wrapper.The Fragment (
<>...</>) wrapping a single child element (<div>) is unnecessary.🔎 Suggested fix
{!isCurrentAutoTask && (feature.status === "waiting_approval" || feature.status === "verified") && ( - <> <div className="absolute top-2 right-2 flex items-center gap-1"> {/* ... buttons ... */} </div> - </> )}
192-258: Redundant Fragment wrapper in "In progress" header block.Same issue as the waiting_approval/verified section - the Fragment is not needed around a single
<div>.
291-313: Consider using optional chaining for safer length check.If both
feature.descriptionandfeature.summaryareundefined, the current expression(feature.description || feature.summary || "").lengthworks, but it's slightly inconsistent with how Line 289 displays the content (which also falls back tofeature.id).🔎 Suggested improvement for consistency
- {(feature.description || feature.summary || "").length > 100 && ( + {((feature.description || feature.summary)?.length ?? 0) > 100 && (apps/ui/src/components/views/board-view/components/kanban-card/card-content-sections.tsx (2)
27-52: PR number extraction may produce misleading results for non-standard URLs.The logic
feature.prUrl.split("/").pop()extracts the last path segment. For standard GitHub PR URLs likehttps://github.com/org/repo/pull/123, this works. However, for URLs with trailing slashes or query parameters, it may produce unexpected results (empty string or query string).🔎 More robust extraction
{typeof feature.prUrl === "string" && /^https?:\/\//i.test(feature.prUrl) && (() => { - const prNumber = feature.prUrl.split("/").pop(); + const match = feature.prUrl.match(/\/pull\/(\d+)/); + const prNumber = match?.[1]; return ( // ... <span className="truncate max-w-[150px]"> - {prNumber ? `Pull Request #${prNumber}` : "Pull Request"} + {prNumber ? `Pull Request #${prNumber}` : "View Pull Request"} </span>
57-71: Consider using stable keys for steps list.Using array index as a key (
key={index}) can cause issues if steps are reordered. If steps have unique identifiers or are immutable during render, this is acceptable.apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx (2)
23-29: Consider whetherfeatureprop is needed or if specific fields suffice.The component uses only
feature.id,feature.description, andfeature.summary. Passing the entireFeatureobject works, but passing specific fields would make the component's dependencies more explicit and potentially more reusable.
45-51: IIFE pattern for truncation is fine but could be extracted.This truncation logic is duplicated conceptually. Consider a utility function like
truncateText(text, maxLength)if used elsewhere.apps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsx (1)
169-176: ESLint disable comments suggest environment configuration issue.The
no-undefwarnings forsetInterval/clearIntervalindicate the ESLint environment doesn't recognize browser globals. Consider addingbrowser: trueto your ESLint env config rather than disabling per-line.apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx (1)
15-31: Large number of optional props may indicate need for composition.The interface has 11 optional handler props. While this works, it could be simplified by grouping related actions or using a discriminated union based on feature status.
apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx (2)
48-48: Unused proponMoveBackToInProgress.The prop is received but renamed with underscore prefix and never used in the component. Either remove it from the interface and destructuring, or implement the intended functionality.
🔎 Proposed fix: Remove unused prop
onFollowUp, onImplement, onComplete, onViewPlan, onApprovePlan, - onMoveBackToInProgress: _onMoveBackToInProgress, hasContext,Also remove from the interface (line 22):
- onMoveBackToInProgress?: () => void;
93-101: Consider typed style object instead of type assertions.The
as Record<string, string>casts work but are verbose. A cleaner approach would be to build the style object with proper typing from the start.🔎 Proposed refactor
- const borderStyle: React.CSSProperties = { ...style }; - if (!cardBorderEnabled) { - (borderStyle as Record<string, string>).borderWidth = "0px"; - (borderStyle as Record<string, string>).borderColor = "transparent"; - } else if (cardBorderOpacity !== 100) { - (borderStyle as Record<string, string>).borderWidth = "1px"; - (borderStyle as Record<string, string>).borderColor = - `color-mix(in oklch, var(--border) ${cardBorderOpacity}%, transparent)`; - } + const borderStyle: React.CSSProperties = { + ...style, + ...(!cardBorderEnabled && { + borderWidth: "0px", + borderColor: "transparent", + }), + ...(cardBorderEnabled && cardBorderOpacity !== 100 && { + borderWidth: "1px", + borderColor: `color-mix(in oklch, var(--border) ${cardBorderOpacity}%, transparent)`, + }), + };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/ui/src/components/views/board-view/components/index.ts(1 hunks)apps/ui/src/components/views/board-view/components/kanban-card.tsx(0 hunks)apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx(1 hunks)apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx(1 hunks)apps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsx(1 hunks)apps/ui/src/components/views/board-view/components/kanban-card/card-content-sections.tsx(1 hunks)apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx(1 hunks)apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx(1 hunks)apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx(1 hunks)apps/ui/src/components/views/board-view/kanban-board.tsx(0 hunks)
💤 Files with no reviewable changes (2)
- apps/ui/src/components/views/board-view/kanban-board.tsx
- apps/ui/src/components/views/board-view/components/kanban-card.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx (7)
apps/ui/src/components/views/board-view/components/index.ts (1)
KanbanCard(1-1)apps/ui/src/store/app-store.ts (1)
useAppStore(853-2358)apps/ui/src/components/ui/card.tsx (2)
Card(94-94)CardContent(100-100)apps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsx (1)
CardBadges(48-134)apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx (1)
CardHeaderSection(40-329)apps/ui/src/components/views/board-view/components/kanban-card/card-content-sections.tsx (1)
CardContentSections(10-81)apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx (1)
AgentInfoPanel(46-283)
apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx (1)
apps/ui/src/components/ui/card.tsx (3)
CardHeader(95-95)CardTitle(97-97)CardDescription(99-99)
apps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsx (1)
apps/server/src/lib/dependency-resolver.ts (1)
getBlockingDependencies(209-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: claude-review
🔇 Additional comments (15)
apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx (2)
1-38: LGTM - Clean imports and well-defined interface.The imports are organized and the
CardHeaderPropsinterface clearly defines the component's contract with appropriate optional callback foronViewOutput.
283-290: Good defensive fallback for description display.Using
feature.description || feature.summary || feature.idensures a value is always rendered, which is good UX. The ID fallback is a reasonable last resort for debugging.apps/ui/src/components/views/board-view/components/index.ts (1)
1-2: LGTM - Export path correctly updated.The barrel export maintains the public API while pointing to the new nested directory structure. Consumers importing
KanbanCardfrom this index won't need changes.apps/ui/src/components/views/board-view/components/kanban-card/card-content-sections.tsx (1)
10-81: LGTM - Clean component structure with good conditional rendering.The component properly handles optional sections and uses appropriate accessibility attributes on links.
apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx (1)
30-74: LGTM - Well-structured dialog component.The dialog properly manages open state, has good accessibility with semantic Dialog components, and provides clear user feedback with the Close button.
apps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsx (2)
48-57: LGTM - Proper memoization of blocking dependencies.The
useMemocorrectly depends onenableDependencyBlocking,feature, andfeatures, ensuring recalculation only when necessary.
196-221: Priority badge rendering logic is clear and well-structured.The conditional styling based on priority levels (1=High, 2=Medium, 3=Low) is easy to follow with appropriate visual differentiation.
apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx (2)
50-111: LGTM - Well-structured action buttons for running tasks.The priority ordering (Approve Plan first when available) and proper event handling make this section clear and functional.
285-334: LGTM - Backlog actions are comprehensive.The Edit, View Plan, and Make actions cover the expected backlog workflow with appropriate conditional rendering.
apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx (3)
58-112: LGTM on useEffect structure.The polling interval setup and cleanup logic is correctly implemented. The interval is only created when
isCurrentAutoTaskis true, and the cleanup properly clears it when the effect re-runs or the component unmounts.
27-37: LGTM!Clean helper function with correct type handling.
114-135: LGTM!The backlog rendering branch correctly displays model configuration without the summary dialog, which is appropriate since backlog items haven't been processed yet.
apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx (3)
70-91: LGTM!The draggable state logic and
useSortableconfiguration are correctly implemented. Cards are appropriately non-draggable when they're the current auto task (in_progress).
103-206: LGTM!Well-structured composition of subcomponents with clean separation of concerns. The conditional spreading of drag listeners on line 134 correctly prevents drag interactions when the card should not be draggable.
208-214: LGTM!Clean conditional wrapper pattern for the animated border effect on active tasks.
| if (showAgentInfo && feature.status !== "backlog" && agentInfo) { | ||
| return ( | ||
| <div className="mb-3 space-y-2 overflow-hidden"> | ||
| {/* Model & Phase */} | ||
| <div className="flex items-center gap-2 text-[11px] flex-wrap"> | ||
| <div className="flex items-center gap-1 text-[var(--status-info)]"> | ||
| <Cpu className="w-3 h-3" /> | ||
| <span className="font-medium"> | ||
| {formatModelName(feature.model ?? DEFAULT_MODEL)} | ||
| </span> | ||
| </div> | ||
| {agentInfo.currentPhase && ( | ||
| <div | ||
| className={cn( | ||
| "px-1.5 py-0.5 rounded-md text-[10px] font-medium", | ||
| agentInfo.currentPhase === "planning" && | ||
| "bg-[var(--status-info-bg)] text-[var(--status-info)]", | ||
| agentInfo.currentPhase === "action" && | ||
| "bg-[var(--status-warning-bg)] text-[var(--status-warning)]", | ||
| agentInfo.currentPhase === "verification" && | ||
| "bg-[var(--status-success-bg)] text-[var(--status-success)]" | ||
| )} | ||
| > | ||
| {agentInfo.currentPhase} | ||
| </div> | ||
| )} | ||
| </div> | ||
|
|
||
| {/* Task List Progress */} | ||
| {agentInfo.todos.length > 0 && ( | ||
| <div className="space-y-1"> | ||
| <div className="flex items-center gap-1 text-[10px] text-muted-foreground/70"> | ||
| <ListTodo className="w-3 h-3" /> | ||
| <span> | ||
| {agentInfo.todos.filter((t) => t.status === "completed").length} | ||
| /{agentInfo.todos.length} tasks | ||
| </span> | ||
| </div> | ||
| <div className="space-y-0.5 max-h-16 overflow-y-auto"> | ||
| {agentInfo.todos.slice(0, 3).map((todo, idx) => ( | ||
| <div | ||
| key={idx} | ||
| className="flex items-center gap-1.5 text-[10px]" | ||
| > | ||
| {todo.status === "completed" ? ( | ||
| <CheckCircle2 className="w-2.5 h-2.5 text-[var(--status-success)] shrink-0" /> | ||
| ) : todo.status === "in_progress" ? ( | ||
| <Loader2 className="w-2.5 h-2.5 text-[var(--status-warning)] animate-spin shrink-0" /> | ||
| ) : ( | ||
| <Circle className="w-2.5 h-2.5 text-muted-foreground/50 shrink-0" /> | ||
| )} | ||
| <span | ||
| className={cn( | ||
| "break-words hyphens-auto line-clamp-2 leading-relaxed", | ||
| todo.status === "completed" && | ||
| "text-muted-foreground/60 line-through", | ||
| todo.status === "in_progress" && | ||
| "text-[var(--status-warning)]", | ||
| todo.status === "pending" && "text-muted-foreground/80" | ||
| )} | ||
| > | ||
| {todo.content} | ||
| </span> | ||
| </div> | ||
| ))} | ||
| {agentInfo.todos.length > 3 && ( | ||
| <p className="text-[10px] text-muted-foreground/60 pl-4"> | ||
| +{agentInfo.todos.length - 3} more | ||
| </p> | ||
| )} | ||
| </div> | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Summary for waiting_approval and verified */} | ||
| {(feature.status === "waiting_approval" || | ||
| feature.status === "verified") && ( | ||
| <> | ||
| {(feature.summary || summary || agentInfo.summary) && ( | ||
| <div className="space-y-1.5 pt-2 border-t border-border/30 overflow-hidden"> | ||
| <div className="flex items-center justify-between gap-2"> | ||
| <div className="flex items-center gap-1 text-[10px] text-[var(--status-success)] min-w-0"> | ||
| <Sparkles className="w-3 h-3 shrink-0" /> | ||
| <span className="truncate font-medium">Summary</span> | ||
| </div> | ||
| <button | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| setIsSummaryDialogOpen(true); | ||
| }} | ||
| onPointerDown={(e) => e.stopPropagation()} | ||
| className="p-0.5 rounded-md hover:bg-muted/80 transition-colors text-muted-foreground/60 hover:text-muted-foreground shrink-0" | ||
| title="View full summary" | ||
| data-testid={`expand-summary-${feature.id}`} | ||
| > | ||
| <Expand className="w-3 h-3" /> | ||
| </button> | ||
| </div> | ||
| <p className="text-[10px] text-muted-foreground/70 line-clamp-3 break-words hyphens-auto leading-relaxed overflow-hidden"> | ||
| {feature.summary || summary || agentInfo.summary} | ||
| </p> | ||
| </div> | ||
| )} | ||
| {!feature.summary && | ||
| !summary && | ||
| !agentInfo.summary && | ||
| agentInfo.toolCallCount > 0 && ( | ||
| <div className="flex items-center gap-2 text-[10px] text-muted-foreground/60 pt-2 border-t border-border/30"> | ||
| <span className="flex items-center gap-1"> | ||
| <Wrench className="w-2.5 h-2.5" /> | ||
| {agentInfo.toolCallCount} tool calls | ||
| </span> | ||
| {agentInfo.todos.length > 0 && ( | ||
| <span className="flex items-center gap-1"> | ||
| <CheckCircle2 className="w-2.5 h-2.5 text-[var(--status-success)]" /> | ||
| { | ||
| agentInfo.todos.filter((t) => t.status === "completed") | ||
| .length | ||
| }{" "} | ||
| tasks done | ||
| </span> | ||
| )} | ||
| </div> | ||
| )} | ||
| </> | ||
| )} | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
SummaryDialog is not rendered when the expand button is present.
When this branch returns (non-backlog cards with agentInfo), the expand button on lines 223-234 sets isSummaryDialogOpen to true, but the SummaryDialog component is only rendered in the final return block (lines 270-282), which is never reached due to this early return. Clicking the expand button will have no visible effect.
🔎 Proposed fix: Include SummaryDialog in this return block
// Agent Info Panel for non-backlog cards
if (showAgentInfo && feature.status !== "backlog" && agentInfo) {
return (
- <div className="mb-3 space-y-2 overflow-hidden">
+ <>
+ <div className="mb-3 space-y-2 overflow-hidden">
{/* Model & Phase */}
...
{/* Summary for waiting_approval and verified */}
...
- </div>
+ </div>
+ <SummaryDialog
+ feature={feature}
+ agentInfo={agentInfo}
+ summary={summary}
+ isOpen={isSummaryDialogOpen}
+ onOpenChange={setIsSummaryDialogOpen}
+ />
+ </>
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (showAgentInfo && feature.status !== "backlog" && agentInfo) { | |
| return ( | |
| <div className="mb-3 space-y-2 overflow-hidden"> | |
| {/* Model & Phase */} | |
| <div className="flex items-center gap-2 text-[11px] flex-wrap"> | |
| <div className="flex items-center gap-1 text-[var(--status-info)]"> | |
| <Cpu className="w-3 h-3" /> | |
| <span className="font-medium"> | |
| {formatModelName(feature.model ?? DEFAULT_MODEL)} | |
| </span> | |
| </div> | |
| {agentInfo.currentPhase && ( | |
| <div | |
| className={cn( | |
| "px-1.5 py-0.5 rounded-md text-[10px] font-medium", | |
| agentInfo.currentPhase === "planning" && | |
| "bg-[var(--status-info-bg)] text-[var(--status-info)]", | |
| agentInfo.currentPhase === "action" && | |
| "bg-[var(--status-warning-bg)] text-[var(--status-warning)]", | |
| agentInfo.currentPhase === "verification" && | |
| "bg-[var(--status-success-bg)] text-[var(--status-success)]" | |
| )} | |
| > | |
| {agentInfo.currentPhase} | |
| </div> | |
| )} | |
| </div> | |
| {/* Task List Progress */} | |
| {agentInfo.todos.length > 0 && ( | |
| <div className="space-y-1"> | |
| <div className="flex items-center gap-1 text-[10px] text-muted-foreground/70"> | |
| <ListTodo className="w-3 h-3" /> | |
| <span> | |
| {agentInfo.todos.filter((t) => t.status === "completed").length} | |
| /{agentInfo.todos.length} tasks | |
| </span> | |
| </div> | |
| <div className="space-y-0.5 max-h-16 overflow-y-auto"> | |
| {agentInfo.todos.slice(0, 3).map((todo, idx) => ( | |
| <div | |
| key={idx} | |
| className="flex items-center gap-1.5 text-[10px]" | |
| > | |
| {todo.status === "completed" ? ( | |
| <CheckCircle2 className="w-2.5 h-2.5 text-[var(--status-success)] shrink-0" /> | |
| ) : todo.status === "in_progress" ? ( | |
| <Loader2 className="w-2.5 h-2.5 text-[var(--status-warning)] animate-spin shrink-0" /> | |
| ) : ( | |
| <Circle className="w-2.5 h-2.5 text-muted-foreground/50 shrink-0" /> | |
| )} | |
| <span | |
| className={cn( | |
| "break-words hyphens-auto line-clamp-2 leading-relaxed", | |
| todo.status === "completed" && | |
| "text-muted-foreground/60 line-through", | |
| todo.status === "in_progress" && | |
| "text-[var(--status-warning)]", | |
| todo.status === "pending" && "text-muted-foreground/80" | |
| )} | |
| > | |
| {todo.content} | |
| </span> | |
| </div> | |
| ))} | |
| {agentInfo.todos.length > 3 && ( | |
| <p className="text-[10px] text-muted-foreground/60 pl-4"> | |
| +{agentInfo.todos.length - 3} more | |
| </p> | |
| )} | |
| </div> | |
| </div> | |
| )} | |
| {/* Summary for waiting_approval and verified */} | |
| {(feature.status === "waiting_approval" || | |
| feature.status === "verified") && ( | |
| <> | |
| {(feature.summary || summary || agentInfo.summary) && ( | |
| <div className="space-y-1.5 pt-2 border-t border-border/30 overflow-hidden"> | |
| <div className="flex items-center justify-between gap-2"> | |
| <div className="flex items-center gap-1 text-[10px] text-[var(--status-success)] min-w-0"> | |
| <Sparkles className="w-3 h-3 shrink-0" /> | |
| <span className="truncate font-medium">Summary</span> | |
| </div> | |
| <button | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| setIsSummaryDialogOpen(true); | |
| }} | |
| onPointerDown={(e) => e.stopPropagation()} | |
| className="p-0.5 rounded-md hover:bg-muted/80 transition-colors text-muted-foreground/60 hover:text-muted-foreground shrink-0" | |
| title="View full summary" | |
| data-testid={`expand-summary-${feature.id}`} | |
| > | |
| <Expand className="w-3 h-3" /> | |
| </button> | |
| </div> | |
| <p className="text-[10px] text-muted-foreground/70 line-clamp-3 break-words hyphens-auto leading-relaxed overflow-hidden"> | |
| {feature.summary || summary || agentInfo.summary} | |
| </p> | |
| </div> | |
| )} | |
| {!feature.summary && | |
| !summary && | |
| !agentInfo.summary && | |
| agentInfo.toolCallCount > 0 && ( | |
| <div className="flex items-center gap-2 text-[10px] text-muted-foreground/60 pt-2 border-t border-border/30"> | |
| <span className="flex items-center gap-1"> | |
| <Wrench className="w-2.5 h-2.5" /> | |
| {agentInfo.toolCallCount} tool calls | |
| </span> | |
| {agentInfo.todos.length > 0 && ( | |
| <span className="flex items-center gap-1"> | |
| <CheckCircle2 className="w-2.5 h-2.5 text-[var(--status-success)]" /> | |
| { | |
| agentInfo.todos.filter((t) => t.status === "completed") | |
| .length | |
| }{" "} | |
| tasks done | |
| </span> | |
| )} | |
| </div> | |
| )} | |
| </> | |
| )} | |
| </div> | |
| ); | |
| } | |
| if (showAgentInfo && feature.status !== "backlog" && agentInfo) { | |
| return ( | |
| <> | |
| <div className="mb-3 space-y-2 overflow-hidden"> | |
| {/* Model & Phase */} | |
| <div className="flex items-center gap-2 text-[11px] flex-wrap"> | |
| <div className="flex items-center gap-1 text-[var(--status-info)]"> | |
| <Cpu className="w-3 h-3" /> | |
| <span className="font-medium"> | |
| {formatModelName(feature.model ?? DEFAULT_MODEL)} | |
| </span> | |
| </div> | |
| {agentInfo.currentPhase && ( | |
| <div | |
| className={cn( | |
| "px-1.5 py-0.5 rounded-md text-[10px] font-medium", | |
| agentInfo.currentPhase === "planning" && | |
| "bg-[var(--status-info-bg)] text-[var(--status-info)]", | |
| agentInfo.currentPhase === "action" && | |
| "bg-[var(--status-warning-bg)] text-[var(--status-warning)]", | |
| agentInfo.currentPhase === "verification" && | |
| "bg-[var(--status-success-bg)] text-[var(--status-success)]" | |
| )} | |
| > | |
| {agentInfo.currentPhase} | |
| </div> | |
| )} | |
| </div> | |
| {/* Task List Progress */} | |
| {agentInfo.todos.length > 0 && ( | |
| <div className="space-y-1"> | |
| <div className="flex items-center gap-1 text-[10px] text-muted-foreground/70"> | |
| <ListTodo className="w-3 h-3" /> | |
| <span> | |
| {agentInfo.todos.filter((t) => t.status === "completed").length} | |
| /{agentInfo.todos.length} tasks | |
| </span> | |
| </div> | |
| <div className="space-y-0.5 max-h-16 overflow-y-auto"> | |
| {agentInfo.todos.slice(0, 3).map((todo, idx) => ( | |
| <div | |
| key={idx} | |
| className="flex items-center gap-1.5 text-[10px]" | |
| > | |
| {todo.status === "completed" ? ( | |
| <CheckCircle2 className="w-2.5 h-2.5 text-[var(--status-success)] shrink-0" /> | |
| ) : todo.status === "in_progress" ? ( | |
| <Loader2 className="w-2.5 h-2.5 text-[var(--status-warning)] animate-spin shrink-0" /> | |
| ) : ( | |
| <Circle className="w-2.5 h-2.5 text-muted-foreground/50 shrink-0" /> | |
| )} | |
| <span | |
| className={cn( | |
| "break-words hyphens-auto line-clamp-2 leading-relaxed", | |
| todo.status === "completed" && | |
| "text-muted-foreground/60 line-through", | |
| todo.status === "in_progress" && | |
| "text-[var(--status-warning)]", | |
| todo.status === "pending" && "text-muted-foreground/80" | |
| )} | |
| > | |
| {todo.content} | |
| </span> | |
| </div> | |
| ))} | |
| {agentInfo.todos.length > 3 && ( | |
| <p className="text-[10px] text-muted-foreground/60 pl-4"> | |
| {agentInfo.todos.length - 3} more | |
| </p> | |
| )} | |
| </div> | |
| </div> | |
| )} | |
| {/* Summary for waiting_approval and verified */} | |
| {(feature.status === "waiting_approval" || | |
| feature.status === "verified") && ( | |
| <> | |
| {(feature.summary || summary || agentInfo.summary) && ( | |
| <div className="space-y-1.5 pt-2 border-t border-border/30 overflow-hidden"> | |
| <div className="flex items-center justify-between gap-2"> | |
| <div className="flex items-center gap-1 text-[10px] text-[var(--status-success)] min-w-0"> | |
| <Sparkles className="w-3 h-3 shrink-0" /> | |
| <span className="truncate font-medium">Summary</span> | |
| </div> | |
| <button | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| setIsSummaryDialogOpen(true); | |
| }} | |
| onPointerDown={(e) => e.stopPropagation()} | |
| className="p-0.5 rounded-md hover:bg-muted/80 transition-colors text-muted-foreground/60 hover:text-muted-foreground shrink-0" | |
| title="View full summary" | |
| data-testid={`expand-summary-${feature.id}`} | |
| > | |
| <Expand className="w-3 h-3" /> | |
| </button> | |
| </div> | |
| <p className="text-[10px] text-muted-foreground/70 line-clamp-3 break-words hyphens-auto leading-relaxed overflow-hidden"> | |
| {feature.summary || summary || agentInfo.summary} | |
| </p> | |
| </div> | |
| )} | |
| {!feature.summary && | |
| !summary && | |
| !agentInfo.summary && | |
| agentInfo.toolCallCount > 0 && ( | |
| <div className="flex items-center gap-2 text-[10px] text-muted-foreground/60 pt-2 border-t border-border/30"> | |
| <span className="flex items-center gap-1"> | |
| <Wrench className="w-2.5 h-2.5" /> | |
| {agentInfo.toolCallCount} tool calls | |
| </span> | |
| {agentInfo.todos.length > 0 && ( | |
| <span className="flex items-center gap-1"> | |
| <CheckCircle2 className="w-2.5 h-2.5 text-[var(--status-success)]" /> | |
| { | |
| agentInfo.todos.filter((t) => t.status === "completed") | |
| .length | |
| }{" "} | |
| tasks done | |
| </span> | |
| )} | |
| </div> | |
| )} | |
| </> | |
| )} | |
| </div> | |
| <SummaryDialog | |
| feature={feature} | |
| agentInfo={agentInfo} | |
| summary={summary} | |
| isOpen={isSummaryDialogOpen} | |
| onOpenChange={setIsSummaryDialogOpen} | |
| /> | |
| </> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx
around lines 138-266, this return block renders the agent info and the "expand"
button but never renders the SummaryDialog, so opening it does nothing; update
this return to also render the SummaryDialog (same props/state used in the main
return) — i.e., add the SummaryDialog component with
isOpen={isSummaryDialogOpen}, setIsOpen={setIsSummaryDialogOpen}, summary source
(feature.summary || summary || agentInfo.summary), and preserve event
stopPropagation and data-testid so the dialog can actually open when the expand
button is clicked.
| ) : onVerify ? ( | ||
| <Button | ||
| variant="default" | ||
| size="sm" | ||
| className="flex-1 h-7 text-[11px] bg-[var(--status-success)] hover:bg-[var(--status-success)]/90" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onVerify(); | ||
| }} | ||
| onPointerDown={(e) => e.stopPropagation()} | ||
| data-testid={`verify-feature-${feature.id}`} | ||
| > | ||
| <PlayCircle className="w-3 h-3 mr-1" /> | ||
| Resume | ||
| </Button> | ||
| ) : null} |
There was a problem hiding this comment.
Button label "Resume" is misleading when using onVerify handler.
This button displays "Resume" but calls onVerify. The previous branch (lines 146-160) correctly shows "Resume" with onResume. This branch should likely have a different label like "Verify" to match its action.
🔎 Suggested fix
) : onVerify ? (
<Button
variant="default"
size="sm"
className="flex-1 h-7 text-[11px] bg-[var(--status-success)] hover:bg-[var(--status-success)]/90"
onClick={(e) => {
e.stopPropagation();
onVerify();
}}
onPointerDown={(e) => e.stopPropagation()}
- data-testid={`verify-feature-${feature.id}`}
+ data-testid={`run-tests-${feature.id}`}
>
<PlayCircle className="w-3 h-3 mr-1" />
- Resume
+ Run Tests
</Button>Alternatively, if "Resume" is the intended UX term regardless of handler, consider renaming the handler for clarity.
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx
around lines 161 to 176, the button text reads "Resume" but invokes the onVerify
handler; change the visible label to "Verify" to match the handler (or
alternatively rename the handler to onResume if the UX should remain "Resume")
so the action and label align; keep existing props (event stopPropagation
handlers, data-testid, icon, classes) and only update the displayed text (and
update data-testid if you rename the handler to maintain test clarity).
| useEffect(() => { | ||
| if (!feature.justFinishedAt || feature.status !== "waiting_approval") { | ||
| return; | ||
| } | ||
|
|
||
| const finishedTime = new Date(feature.justFinishedAt).getTime(); | ||
| const twoMinutes = 2 * 60 * 1000; | ||
| const timeRemaining = twoMinutes - (currentTime - finishedTime); | ||
|
|
||
| if (timeRemaining <= 0) { | ||
| return; | ||
| } | ||
|
|
||
| // eslint-disable-next-line no-undef | ||
| const interval = setInterval(() => { | ||
| setCurrentTime(Date.now()); | ||
| }, 1000); | ||
|
|
||
| return () => { | ||
| // eslint-disable-next-line no-undef | ||
| clearInterval(interval); | ||
| }; | ||
| }, [feature.justFinishedAt, feature.status, currentTime]); |
There was a problem hiding this comment.
Potential issue: currentTime in dependency array causes interval recreation.
Including currentTime in the effect's dependency array means the effect re-runs every time currentTime changes (every second). This causes the interval to be cleared and recreated every second, which is inefficient.
🔎 Suggested fix
useEffect(() => {
if (!feature.justFinishedAt || feature.status !== "waiting_approval") {
return;
}
const finishedTime = new Date(feature.justFinishedAt).getTime();
const twoMinutes = 2 * 60 * 1000;
- const timeRemaining = twoMinutes - (currentTime - finishedTime);
+ const timeRemaining = twoMinutes - (Date.now() - finishedTime);
if (timeRemaining <= 0) {
return;
}
// eslint-disable-next-line no-undef
const interval = setInterval(() => {
setCurrentTime(Date.now());
}, 1000);
return () => {
// eslint-disable-next-line no-undef
clearInterval(interval);
};
- }, [feature.justFinishedAt, feature.status, currentTime]);
+ }, [feature.justFinishedAt, feature.status]);This way the interval is set up once when the feature enters waiting_approval state and cleaned up when the time expires or status changes.
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/board-view/components/kanban-card/card-badges.tsx
around lines 156 to 178, the useEffect recreates the interval every second
because currentTime is included in the dependency array; remove currentTime from
the deps so the effect only runs when feature.justFinishedAt or feature.status
changes, set up a single interval inside the effect that calls
setCurrentTime(Date.now()) each second, and keep the existing early returns and
cleanup (clearInterval) so the interval is created once when the feature enters
waiting_approval and is cleared when status changes or the component unmounts;
if you need to stop the interval when the remaining time elapses, compute
timeRemaining inside the interval and clearInterval when it <= 0.
- Renamed the Commit button to Mark as Verified in the test cases to align with recent UI changes. - Updated feature descriptions in the tests to match the new functionality. - Adjusted visibility checks for the Mark as Verified button to ensure accurate testing of the updated UI behavior.
Code Review: Refactor Kanban CardsThank you for this refactoring PR! This is a well-executed component decomposition that significantly improves maintainability. Strengths1. Excellent Component DecompositionThe refactoring from a monolithic 1,332-line file into smaller, focused components follows clean code principles perfectly. Each new component has a clear, focused purpose with improved maintainability and better code organization. 2. Minimal Breaking ChangesExport path updated cleanly in index.ts, public API remains unchanged, and test changes are minimal. 3. Good State ManagementState appropriately co-located with components that use it. 4. Clean Separation of Concerns
Issues and Concerns1. Bug: Removed Commit Handler from In-Progress CardsLocation: apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx:110-111 The PR description states removed commit handler from in-progress task cards - however, the onCommit prop is still defined in the interface but never used. This is dead code that could confuse developers. Recommendation: Remove the onCommit prop from KanbanCardProps interface if commit functionality was intentionally removed. 2. ESLint Disable CommentsLocation: Multiple files (agent-info-panel.tsx, card-badges.tsx) Multiple eslint-disable-next-line no-undef comments suggest missing type definitions. setInterval, clearInterval, console, and window should be available in DOM environment. Recommendation: Review TypeScript configuration and add proper type definitions instead of disabling lints. 3. Type Safety ConcernsLocation: agent-info-panel.tsx:74 Using any for window.__currentProject defeats TypeScript purpose. Consider creating proper type definitions. 4. Unused ParameterLocation: kanban-card.tsx:48 Parameter onMoveBackToInProgress is prefixed with underscore but never used. Remove from props if not needed. 5. Magic NumbersLocation: card-badges.tsx:152, 163 const twoMinutes = 2 * 60 * 1000 is duplicated across files. Consider extracting to a shared constant. 6. Border Style Type AssertionsLocation: kanban-card.tsx:95-100 Mutating object with type assertions is error-prone. Consider using object spread pattern instead. Code Quality AssessmentAdherence to Clean Code Guidelines
Overall Quality
Security ConsiderationsNo security concerns identified. The refactoring maintains the same security posture as the original implementation. Performance ConsiderationsPositive:
Potential Concerns:
Test CoverageTest changes are minimal and appropriate. Updated test description from Commit to Mark as Verified which reflects the actual functionality being tested. Recommendations SummaryHigh Priority:
Medium Priority: Low Priority: ConclusionThis is a high-quality refactoring that significantly improves the codebase. The decomposition is logical and follows clean code principles. The issues identified are relatively minor and do not detract from the overall value of this change. Recommendation: Approve with minor fixes The identified issues should be addressed (particularly the onCommit handler clarification and type safety improvements), but they do not block merging if the team is comfortable addressing them in follow-up PRs. Great work on improving the maintainability of this critical component! |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.