Conversation
- TaskCard: replace title attr with Tooltip explaining dependency semantics
- TaskDetailModal: full dependency list with task names, status badges, and
navigation via onOpenTask prop (closes + opens dependency's modal)
- Incomplete dependencies (not DONE/MERGED) highlighted in amber
- TaskBoardView: passes onOpenTask={handleTaskClick} to TaskDetailModal
- useSWR for all tasks in modal (reuses TaskBoardView's cache — no extra request)
WalkthroughTask dependency visibility and navigation were added: Changes
Sequence DiagramsequenceDiagram
participant User
participant TaskBoardView
participant TaskDetailModal
participant API
User->>TaskBoardView: Click task to open modal
TaskBoardView->>TaskDetailModal: Open modal (provides onOpenTask)
TaskDetailModal->>API: useSWR -> GET /api/v2/tasks?path=...
API-->>TaskDetailModal: TaskListResponse
TaskDetailModal->>TaskDetailModal: build tasksById and render Dependencies
User->>TaskDetailModal: Click dependency name
TaskDetailModal->>TaskDetailModal: onClose()
TaskDetailModal->>TaskBoardView: onOpenTask(depId)
TaskBoardView->>TaskBoardView: set detailTaskId = depId
TaskBoardView->>TaskDetailModal: reopen modal for depId
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
web-ui/src/components/tasks/TaskCard.tsx (1)
101-113: Consider hoistingTooltipProviderto a parent component.Wrapping each
Tooltipwith its ownTooltipProviderworks but is suboptimal when rendering manyTaskCardcomponents. Shadcn/Radix recommends a singleTooltipProvidernear the app root or at the list level for better performance and consistent delay behavior across tooltips.This is a minor optimization—current implementation is functionally correct.
♻️ Suggested refactor
Move
TooltipProviderto wrap the task list inTaskBoardContentor at the app layout level, then simplify each card:- <TooltipProvider> - <Tooltip> - <TooltipTrigger asChild> - <span className="flex cursor-default items-center gap-1 text-xs text-muted-foreground"> - <LinkCircleIcon className="h-3.5 w-3.5" /> - {task.depends_on.length} - </span> - </TooltipTrigger> - <TooltipContent> - Depends on {task.depends_on.length} task{task.depends_on.length !== 1 ? 's' : ''}. This task will become READY when all dependencies complete. - </TooltipContent> - </Tooltip> - </TooltipProvider> + <Tooltip> + <TooltipTrigger asChild> + <span className="flex cursor-default items-center gap-1 text-xs text-muted-foreground"> + <LinkCircleIcon className="h-3.5 w-3.5" /> + {task.depends_on.length} + </span> + </TooltipTrigger> + <TooltipContent> + Depends on {task.depends_on.length} task{task.depends_on.length !== 1 ? 's' : ''}. This task will become READY when all dependencies complete. + </TooltipContent> + </Tooltip>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/tasks/TaskCard.tsx` around lines 101 - 113, The TaskCard component currently wraps each tooltip in its own TooltipProvider which is inefficient for many TaskCard instances; hoist the TooltipProvider out of TaskCard and place a single shared provider at a higher level (e.g., wrap the task list in TaskBoardContent or the app layout) so all Tooltip components inside TaskCard reuse the same context; then remove the TooltipProvider wrapper from TaskCard and keep Tooltip, TooltipTrigger, TooltipContent usage unchanged to preserve behavior.web-ui/src/components/tasks/TaskDetailModal.tsx (2)
211-217: Consider addingtype="button"to prevent form submission context issues.While this button isn't inside a form currently, explicitly setting
type="button"is a defensive best practice that prevents unintended form submissions if the component hierarchy changes in the future.♻️ Suggested fix
{onOpenTask ? ( <button + type="button" className={`truncate text-left hover:underline ${isIncomplete ? 'font-medium text-amber-700 dark:text-amber-400' : 'text-foreground'}`} onClick={() => { onClose(); onOpenTask(depId); }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/tasks/TaskDetailModal.tsx` around lines 211 - 217, The button rendered in TaskDetailModal (the element using onOpenTask, onClose, depId and dep?.title) should explicitly set type="button" to avoid accidental form submission if the component is ever nested in a form; update the JSX for the button that calls onClose(); onOpenTask(depId) to include type="button" on the element so it behaves as a non-submitting control.
78-83: MemoizetasksByIdMap to avoid recreation on every render.The
tasksByIdMap is recreated on each render. While not expensive for small task lists, memoizing it prevents unnecessary object allocations when unrelated state changes trigger re-renders.♻️ Suggested refactor using useMemo
+'use client'; + +import Link from 'next/link'; +import { useEffect, useState, useMemo } from 'react'; ... // Fetch all tasks (reuses TaskBoardView's SWR cache — same key, no extra network call) const { data: allTasksData } = useSWR<TaskListResponse>( `/api/v2/tasks?path=${workspacePath}`, () => tasksApi.getAll(workspacePath) ); - const tasksById = new Map(allTasksData?.tasks.map((t) => [t.id, t]) ?? []); + const tasksById = useMemo( + () => new Map(allTasksData?.tasks.map((t) => [t.id, t]) ?? []), + [allTasksData?.tasks] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/tasks/TaskDetailModal.tsx` around lines 78 - 83, The tasksById Map is recreated on every render; wrap its construction in useMemo so it only rebuilds when the underlying data changes. Replace the direct const tasksById = new Map(...) with a memoized version that uses React's useMemo and depends on allTasksData (or allTasksData.tasks) so the Map is only reconstructed when the fetched tasks change; reference the existing symbols useSWR, allTasksData, and tasksById to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/src/components/tasks/TaskDetailModal.tsx`:
- Around line 193-228: The dependency navigation button in TaskDetailModal (the
button inside task.depends_on.map that calls onClose() and onOpenTask(depId))
lacks explicit focus-visible styling; update that button to include the same
focus-visible classes used elsewhere (e.g., focus-visible:ring-[3px]
focus-visible:ring-ring and any focus-visible:outline-none or rounded focus
classes) so keyboard focus is clearly visible and matches other interactive
components, ensuring the addition does not change the existing hover/truncate
styles or behavior when onOpenTask is present.
---
Nitpick comments:
In `@web-ui/src/components/tasks/TaskCard.tsx`:
- Around line 101-113: The TaskCard component currently wraps each tooltip in
its own TooltipProvider which is inefficient for many TaskCard instances; hoist
the TooltipProvider out of TaskCard and place a single shared provider at a
higher level (e.g., wrap the task list in TaskBoardContent or the app layout) so
all Tooltip components inside TaskCard reuse the same context; then remove the
TooltipProvider wrapper from TaskCard and keep Tooltip, TooltipTrigger,
TooltipContent usage unchanged to preserve behavior.
In `@web-ui/src/components/tasks/TaskDetailModal.tsx`:
- Around line 211-217: The button rendered in TaskDetailModal (the element using
onOpenTask, onClose, depId and dep?.title) should explicitly set type="button"
to avoid accidental form submission if the component is ever nested in a form;
update the JSX for the button that calls onClose(); onOpenTask(depId) to include
type="button" on the element so it behaves as a non-submitting control.
- Around line 78-83: The tasksById Map is recreated on every render; wrap its
construction in useMemo so it only rebuilds when the underlying data changes.
Replace the direct const tasksById = new Map(...) with a memoized version that
uses React's useMemo and depends on allTasksData (or allTasksData.tasks) so the
Map is only reconstructed when the fetched tasks change; reference the existing
symbols useSWR, allTasksData, and tasksById to locate and update the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7666cb47-3ab4-4b7b-b190-1918072634a2
📒 Files selected for processing (3)
web-ui/src/components/tasks/TaskBoardView.tsxweb-ui/src/components/tasks/TaskCard.tsxweb-ui/src/components/tasks/TaskDetailModal.tsx
|
test comment |
|
PR Review summary: Well-scoped PR. Three issues to address before merge: (1) TooltipProvider mounted per card - should be lifted to board level; (2) SWR key coupling is implicit - extract shared constant; (3) Navigation button clickable when dep not found - disable with tooltip. Nice-to-have follow-ups: as-never cast, loading state, missing title attr, UI component tests. Strengths: proper Radix Tooltip usage, amber affordance for incomplete deps, backward-compatible optional prop, well-scoped 3-file change. |
|
PR Review: Task Dependency Graph Visualization in Detail Modal Well-scoped PR with a clear UX improvement. Replacing raw ID truncation with resolved titles, status badges, and clickable navigation is a meaningful upgrade. IMPORTANT - should address before merge:
NICE-TO-HAVE (track as follow-ups): STRENGTHS: The three important items above should be addressed before merge. Items 4-7 can be tracked as follow-up issues. |
|
Review: feat(web-ui): task dependency graph visualization Good implementation overall. The UX improvement is clear and the zero-extra-network-request approach via SWR cache reuse is the right call. A few items to address before merging. Issue 1: tasksById map rebuilt on every render Without useMemo, a new Map is allocated on every render even when allTasksData has not changed. Wrap it in useMemo with [allTasksData] dependency to fix this. Issue 2: type=button missing on the dependency navigation button Without an explicit type, the button defaults to type=submit inside a dialog, which can trigger unintended form submission in some browsers. Add type='button' to the button element. Issue 3 (nice-to-have): as never cast on Badge variant The cast STATUS_BADGE_VARIANT[dep.status] as never silences TypeScript entirely. Fixing the STATUS_BADGE_VARIANT return type or using a named cast is cleaner and preserves type safety. Minor observations SWR key must exactly match TaskBoardView key for cache sharing. The key /api/v2/tasks?path=workspacePath looks correct - worth noting for future refactors. When allTasksData is loading, dependencies show truncated IDs - acceptable graceful degradation. isIncomplete correctly skips unknown deps when dep is undefined - intentional behavior, just noting it. What works well Tooltip upgrade in TaskCard is clean with proper TooltipProvider wrapping and correct singular/plural. Dependency navigation (close current modal then open target) is the right UX flow. Amber highlight for incomplete dependencies is a good visual cue. Replacing truncated IDs with titled, badged entries is a clear UX win. Items 1 and 2 are worth fixing before merge. Item 3 is a nice-to-have. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web-ui/src/components/tasks/TaskDetailModal.tsx (1)
211-217: Use the sharedButtonprimitive for dependency navigation.Lines 211-217 hand-roll another interactive control inside a Shadcn modal. Switching this to the shared
Buttonkeeps focus, spacing, and token usage aligned with the Nova preset instead of maintaining one-off button styling here.♻️ Proposed refactor
- {onOpenTask ? ( - <button - className={`truncate text-left hover:underline focus-visible:outline-none focus-visible:ring-[3px] focus-visible:ring-ring ${isIncomplete ? 'font-medium text-amber-700 dark:text-amber-400' : 'text-foreground'}`} - onClick={() => { onClose(); onOpenTask(depId); }} - > - {dep?.title ?? depId.slice(0, 12)} - </button> + {onOpenTask ? ( + <Button + type="button" + variant="ghost" + size="sm" + className={`h-auto justify-start px-0 py-0 text-xs hover:bg-transparent hover:underline ${isIncomplete ? 'font-medium text-amber-700 dark:text-amber-400' : 'text-foreground'}`} + onClick={() => { onClose(); onOpenTask(depId); }} + > + <span className="truncate">{dep?.title ?? depId.slice(0, 12)}</span> + </Button> ) : (As per coding guidelines,
Web UI must use Shadcn/UI (Nova preset) with gray color scheme and Hugeicons (@hugeicons/react); never use lucide-react.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/tasks/TaskDetailModal.tsx` around lines 211 - 217, Replace the hand-rolled <button> in TaskDetailModal (the element that calls onClose(); onOpenTask(depId)) with the shared Button primitive so focus, spacing and tokens follow the Shadcn/UI Nova preset; keep the same click behavior (call onClose() then onOpenTask(depId)), propagate the existing conditional styling (isIncomplete => use the appropriate variant/intent or className on Button) and preserve the displayed label (dep?.title ?? depId.slice(0,12)); also ensure the Button import is the shared one used across the app (Nova gray scheme) and use Hugeicons for any iconography instead of lucide-react.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/src/components/tasks/TaskDetailModal.tsx`:
- Around line 78-84: The modal currently only renders task.depends_on (upstream
blockers) and lacks downstream dependents and an explicit empty state; use the
workspace task list (allTasksData and tasksById) to compute reverse edges by
finding tasks where t.depends_on includes the current task.id and render them as
"blocked by" / "blocks" sections inside TaskDetailModal (same component that
references tasksById and task.depends_on), showing an explicit empty-state
message when neither upstream nor downstream exist; update the rendering logic
around the block that reads task.depends_on (lines ~193–228) to derive
downstreamIds = allTasksData.tasks.filter(t =>
t.depends_on?.includes(task.id)).map(t => t.id) and display corresponding task
entries from tasksById for both directions.
---
Nitpick comments:
In `@web-ui/src/components/tasks/TaskDetailModal.tsx`:
- Around line 211-217: Replace the hand-rolled <button> in TaskDetailModal (the
element that calls onClose(); onOpenTask(depId)) with the shared Button
primitive so focus, spacing and tokens follow the Shadcn/UI Nova preset; keep
the same click behavior (call onClose() then onOpenTask(depId)), propagate the
existing conditional styling (isIncomplete => use the appropriate variant/intent
or className on Button) and preserve the displayed label (dep?.title ??
depId.slice(0,12)); also ensure the Button import is the shared one used across
the app (Nova gray scheme) and use Hugeicons for any iconography instead of
lucide-react.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fed3fa1-c81a-4b2e-b653-5d182f240b19
📒 Files selected for processing (1)
web-ui/src/components/tasks/TaskDetailModal.tsx
| // Fetch all tasks (reuses TaskBoardView's SWR cache — same key, no extra network call) | ||
| const { data: allTasksData } = useSWR<TaskListResponse>( | ||
| `/api/v2/tasks?path=${workspacePath}`, | ||
| () => tasksApi.getAll(workspacePath) | ||
| ); | ||
| const tasksById = new Map(allTasksData?.tasks.map((t) => [t.id, t]) ?? []); | ||
|
|
There was a problem hiding this comment.
Render downstream dependencies too.
At Line 193 this still only shows task.depends_on. Issue #476 requires both blockers and tasks blocked by the current task, and the current conditional also gives no explicit empty state when neither side exists. Since allTasksData already contains the full workspace task list, this modal has enough data to derive the reverse edges here as well.
🧩 Implementation sketch
const tasksById = new Map(allTasksData?.tasks.map((t) => [t.id, t]) ?? []);
+ const blockedTasks = task
+ ? (allTasksData?.tasks.filter((candidate) => candidate.depends_on.includes(task.id)) ?? [])
+ : [];
...
- {task.depends_on.length > 0 && (
- <div className="space-y-1.5">
+ {(task.depends_on.length > 0 || blockedTasks.length > 0) ? (
+ <div className="space-y-3">
...
+ <div className="flex items-center gap-1.5 text-xs font-medium text-muted-foreground">
+ <LinkCircleIcon className="h-3.5 w-3.5" />
+ Blocked by this task ({blockedTasks.length})
+ </div>
+ <ul className="space-y-1">
+ {blockedTasks.map((blockedTask) => (
+ <li key={blockedTask.id}>...</li>
+ ))}
+ </ul>
</div>
+ ) : (
+ <p className="text-xs italic text-muted-foreground">No dependencies.</p>
)}Also applies to: 193-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/tasks/TaskDetailModal.tsx` around lines 78 - 84, The
modal currently only renders task.depends_on (upstream blockers) and lacks
downstream dependents and an explicit empty state; use the workspace task list
(allTasksData and tasksById) to compute reverse edges by finding tasks where
t.depends_on includes the current task.id and render them as "blocked by" /
"blocks" sections inside TaskDetailModal (same component that references
tasksById and task.depends_on), showing an explicit empty-state message when
neither upstream nor downstream exist; update the rendering logic around the
block that reads task.depends_on (lines ~193–228) to derive downstreamIds =
allTasksData.tasks.filter(t => t.depends_on?.includes(task.id)).map(t => t.id)
and display corresponding task entries from tasksById for both directions.
1 participant
Summary
<Tooltip>explaining semantics — "Depends on N task(s). This task will become READY when all dependencies complete."onOpenTask)useSWRreusesTaskBoardView's cached/api/v2/tasksresponse — zero extra network requestsonOpenTask={handleTaskClick}to wire dependency navigationCloses #476
Test plan
Summary by CodeRabbit
New Features
Improvements