feat: Add task dependencies and spawn sub-task functionality#243
feat: Add task dependencies and spawn sub-task functionality#243webdevcody merged 6 commits intoAutoMaker-Org:mainfrom
Conversation
|
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. 📝 WalkthroughWalkthroughIntroduces spawn sub-task functionality across UI components with dependency management capabilities. Adds collapsible component dependency, enhances dialogs/boards/graphs with spawn callbacks, implements ancestor context display, and adds utilities for circular dependency detection and dependency validation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant KanbanBoard as Kanban View
participant Dialog as Add Feature<br/>Dialog
participant BoardView as Board View<br/>(State)
participant AddFeatureAPI as Feature API
User->>KanbanBoard: Click "Spawn Sub-Task"
KanbanBoard->>BoardView: onSpawnTask(parentFeature)
activate BoardView
BoardView->>BoardView: setSpawnParentFeature(feature)
BoardView->>Dialog: open with parentFeature
deactivate BoardView
User->>Dialog: Select ancestors<br/>Enter description
User->>Dialog: Click "Spawn Task"
activate Dialog
Dialog->>Dialog: formatAncestorContextForPrompt()
Dialog->>Dialog: Prepend context to description
Dialog->>AddFeatureAPI: onAdd({...featureData, dependencies: [parentId]})
deactivate Dialog
AddFeatureAPI->>BoardView: Feature created with dependency
BoardView->>KanbanBoard: Refresh board
User-->>KanbanBoard: New sub-task appears
sequenceDiagram
actor User
participant GraphView as Graph View
participant GraphCanvas as Graph Canvas<br/>(React Flow)
participant ValidationFn as Validation Logic
participant UpdateAPI as Update Feature<br/>API
participant Toast as Toast Notifier
User->>GraphView: Drag edge from Node A → Node B
GraphView->>GraphCanvas: Edge connection event
activate GraphCanvas
GraphCanvas->>ValidationFn: isValidConnection(A, B)
ValidationFn-->>GraphCanvas: true/false
deactivate GraphCanvas
alt Valid Connection
GraphCanvas->>GraphView: onCreateDependency(A, B)
activate GraphView
GraphView->>ValidationFn: wouldCreateCircularDependency()
ValidationFn-->>GraphView: false (safe)
GraphView->>ValidationFn: dependencyExists(A, B)
ValidationFn-->>GraphView: false (new)
GraphView->>UpdateAPI: onUpdateFeature(B, {dependencies: [A]})
UpdateAPI-->>GraphView: Feature B updated
GraphView->>Toast: "Dependency created"
deactivate GraphView
else Invalid (Self-link/Cycle/Duplicate)
GraphView->>Toast: "Cannot create dependency"
end
User->>GraphView: Hover over edge → Click delete
GraphView->>UpdateAPI: Remove dependency from target
Toast->>User: "Dependency removed"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @JBotwina, 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 enhances task management capabilities by introducing visual dependency creation in the graph view and a streamlined process for spawning sub-tasks. These features aim to improve task organization, facilitate the breakdown of complex work, and ensure that new tasks are created with relevant historical context, ultimately leading to a more efficient and intuitive workflow. 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 introduces a new 'Spawn Sub-Task' feature, enabling users to create new tasks that are automatically linked as dependencies to an existing parent task. Key changes include adding a new Collapsible UI component using Radix UI, integrating 'Spawn Sub-Task' actions into Kanban cards and graph nodes, and updating the AddFeatureDialog to support a 'spawn mode'. In spawn mode, the dialog now displays an 'Ancestor Context' section, allowing users to select which parent and ancestor information should be included in the new sub-task's description. The system also now automatically adds the parent as a dependency and includes new utility functions for traversing the dependency graph (getAncestors), formatting ancestor context for prompts, and validating dependencies to prevent self-dependencies, duplicate dependencies, and circular dependencies. Review comments highlight the need for more immediate visual feedback for dependency validation (e.g., preventing edge drawing or showing temporary red outlines for circular dependencies), suggest improving the onOpenChange logic for AddFeatureDialog to prevent stale data, and recommend extracting complex description logic into a helper function for better readability. Additionally, there are suggestions to use a more robust truncation utility for feature titles/descriptions and to refine the cycle detection logic in wouldCreateCircularDependency and getAncestors by correctly managing visited and recursionStack states to ensure accurate cycle identification.
apps/ui/src/components/views/graph-view/utils/ancestor-context.ts
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/graph-view/utils/dependency-validation.ts
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/shared/ancestor-context-section.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/graph-view/utils/ancestor-context.ts
Outdated
Show resolved
Hide resolved
![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)
apps/ui/src/components/ui/collapsible.tsx (1)
1-10: Unused React import.The
Reactimport on line 1 is not used in this file since there's no JSX or React API usage—only re-exports of Radix primitives.🔎 Proposed fix
-import * as React from 'react'; import * as CollapsiblePrimitive from '@radix-ui/react-collapsible'; const Collapsible = CollapsiblePrimitive.Root; const CollapsibleTrigger = CollapsiblePrimitive.CollapsibleTrigger; const CollapsibleContent = CollapsiblePrimitive.CollapsibleContent; export { Collapsible, CollapsibleTrigger, CollapsibleContent };apps/ui/src/components/views/board-view/shared/ancestor-context-section.tsx (1)
108-108: Consider accessibility improvement for the scrollable region.The ancestor list is scrollable but lacks an ARIA label. Consider adding
aria-label="Ancestor context list"orrole="listbox"for better screen reader support.🔎 Proposed fix
- <div className="space-y-1 max-h-[200px] overflow-y-auto border rounded-lg p-2 bg-muted/20"> + <div className="space-y-1 max-h-[200px] overflow-y-auto border rounded-lg p-2 bg-muted/20" aria-label="Ancestor context list">apps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsx (1)
52-56: Consider relocating shared utilities.The ancestor context utilities are imported from
graph-view/utilsinto aboard-viewcomponent. Since these utilities are now shared across views, consider moving them to a common location (e.g.,shared/utilsorlib/) to clarify the dependency direction.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
apps/ui/package.jsonapps/ui/src/components/ui/collapsible.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/components/kanban-card/card-header.tsxapps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsxapps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsxapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/board-view/kanban-board.tsxapps/ui/src/components/views/board-view/shared/ancestor-context-section.tsxapps/ui/src/components/views/board-view/shared/index.tsapps/ui/src/components/views/graph-view/components/task-node.tsxapps/ui/src/components/views/graph-view/graph-canvas.tsxapps/ui/src/components/views/graph-view/graph-view.tsxapps/ui/src/components/views/graph-view/hooks/use-graph-nodes.tsapps/ui/src/components/views/graph-view/utils/ancestor-context.tsapps/ui/src/components/views/graph-view/utils/dependency-validation.tsapps/ui/src/components/views/graph-view/utils/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/ui/src/components/views/graph-view/graph-view.tsx (2)
apps/ui/src/store/app-store.ts (1)
Feature(256-272)apps/ui/src/components/views/graph-view/utils/dependency-validation.ts (2)
dependencyExists(47-51)wouldCreateCircularDependency(12-37)
apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx (1)
apps/ui/src/components/ui/button.tsx (1)
Button(108-108)
apps/ui/src/components/views/board-view.tsx (2)
apps/ui/src/store/app-store.ts (1)
Feature(256-272)libs/types/src/feature.ts (1)
Feature(24-58)
apps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsx (4)
apps/ui/src/store/app-store.ts (1)
Feature(256-272)libs/types/src/feature.ts (1)
Feature(24-58)apps/ui/src/components/views/graph-view/utils/ancestor-context.ts (3)
AncestorContext(3-10)getAncestors(21-57)formatAncestorContextForPrompt(66-93)apps/ui/src/components/views/board-view/shared/ancestor-context-section.tsx (1)
AncestorContextSection(25-201)
🔇 Additional comments (46)
apps/ui/package.json (1)
47-47: LGTM!The
@radix-ui/react-collapsibledependency is appropriate for the ancestor context collapsible UI. The version range follows the same pattern as other Radix packages in this project.apps/ui/src/components/views/board-view/hooks/use-board-actions.ts (2)
101-101: LGTM!The optional
dependenciesparameter with appropriate default handling enables the spawn sub-task feature to pass dependency relationships through the feature creation flow.
154-154: LGTM!Correctly defaults to an empty array when dependencies aren't provided, ensuring consistent data shape.
apps/ui/src/components/views/graph-view/utils/ancestor-context.ts (2)
21-57: LGTM!The ancestor traversal implementation is well-designed:
- Uses a
visitedSet to prevent cyclesmaxDepthparameter guards against runaway recursionfeatureMapprovides O(1) lookups- Sorting by depth ensures closest ancestors appear first
66-93: LGTM!Clean markdown generation with sensible fallback for missing titles. The structured output format will work well for inclusion in task prompts.
apps/ui/src/components/views/board-view/shared/index.ts (1)
9-9: LGTM!Standard barrel export addition for the new
AncestorContextSectioncomponent.apps/ui/src/components/views/graph-view/components/task-node.tsx (1)
270-279: LGTM!The "Spawn Sub-Task" menu item follows the established pattern of other dropdown actions. The
GitForkicon is semantically appropriate, and the event handling withstopPropagationis correct.apps/ui/src/components/views/graph-view/utils/index.ts (1)
1-2: LGTM!Clean barrel file establishing the graph-view utilities export surface.
apps/ui/src/components/views/board-view/kanban-board.tsx (2)
44-44: LGTM!The optional
onSpawnTaskcallback prop follows the established pattern for action handlers in this component.
189-189: LGTM!Correctly wraps the callback to pass the feature context, with optional chaining for safe invocation when the prop isn't provided.
apps/ui/src/components/views/graph-view/graph-canvas.tsx (4)
14-14: LGTM!Connection type import is correctly added to support the new edge connection handling.
57-57: LGTM!The onCreateDependency prop is well-designed with an optional callback pattern that returns a Promise to indicate success/failure.
Also applies to: 69-69
144-155: LGTM!The edge connection handler is well-implemented with proper validation and clear documentation of React Flow's source/target semantics.
184-184: LGTM!The handleConnect callback is correctly wired to ReactFlow's onConnect prop, completing the dependency creation flow.
apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx (1)
28-28: LGTM!The onSpawnTask callback is correctly threaded through KanbanCard to CardHeaderSection, consistent with the existing prop pattern.
Also applies to: 55-55, 151-151
apps/ui/src/components/views/graph-view/hooks/use-graph-nodes.ts (1)
27-27: LGTM!The onSpawnTask callback follows the established pattern for other action callbacks with proper feature ID binding.
Also applies to: 44-44, 117-119
apps/ui/src/components/views/board-view.tsx (5)
110-111: LGTM!State variable for tracking the parent feature during spawn operations is appropriately named and initialized.
1027-1030: LGTM!Both KanbanBoard and GraphView spawn handlers correctly set the parent feature and open the add dialog for a unified spawn experience.
Also applies to: 1056-1059
1094-1099: LGTM!Clearing the spawn parent feature when the dialog closes prevents stale state and ensures clean spawn operations.
1110-1111: LGTM!Passing parentFeature and allFeatures to AddFeatureDialog enables the ancestor context selection UI for spawned tasks.
1053-1055: Verify thathandleUpdateFeaturepersists changes to the backend.The
onUpdateFeaturecallback is essential for dependency creation to work correctly. Ensure thathandleUpdateFeature(fromuseBoardActions) properly persists feature updates to the backend and does not only update local state.apps/ui/src/components/views/board-view/shared/ancestor-context-section.tsx (5)
25-61: LGTM!The event handlers for toggling selection and expansion are correctly implemented using Set operations for efficient state management.
62-68: LGTM!The combined list construction correctly marks the parent with depth -1 and isParent true, while ancestors use their own depth values. This enables proper indentation rendering.
116-118: Fallback logic for displayTitle is well-designed.The title fallback gracefully handles missing titles by truncating the description, ensuring the UI always displays meaningful text.
127-127: Dynamic indentation calculation is correct.Using
item.depth * 12for ancestor indentation while parent has 0 margin creates a clear visual hierarchy.
193-197: LGTM!The empty state message is appropriate and informs users when there are no ancestors beyond the parent.
apps/ui/src/components/views/graph-view/utils/dependency-validation.ts (2)
12-37: LGTM!The circular dependency detection correctly uses DFS to check if adding targetId → sourceId would create a cycle by verifying whether sourceId can already reach targetId through existing dependencies. The visited set prevents infinite loops in existing cycles.
47-51: LGTM!The dependency existence check is straightforward and correctly verifies whether targetId already depends on sourceId.
apps/ui/src/components/views/graph-view/graph-view.tsx (4)
6-7: LGTM!Importing the validation utilities and toast notification library is appropriate for the dependency creation flow.
22-23: LGTM!The new props onUpdateFeature and onSpawnTask extend GraphView's capabilities correctly for dependency management and task spawning.
Also applies to: 39-40
159-164: LGTM!The onSpawnTask callback is correctly integrated into nodeActionCallbacks and the dependency array, enabling task spawning from graph nodes.
Also applies to: 166-166
178-178: LGTM!Wiring handleCreateDependency to GraphCanvas completes the end-to-end dependency creation flow via edge dragging.
apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx (5)
22-22: LGTM!GitFork icon import and onSpawnTask prop are correctly added to support the spawn sub-task functionality.
Also applies to: 35-35, 45-45
98-108: LGTM!Spawn Sub-Task action properly integrated into the running task dropdown with appropriate event handling and test ID.
123-137: LGTM!The backlog header correctly accommodates the new spawn button alongside the delete button, with proper event propagation control and accessibility attributes (title, data-testid).
172-187: LGTM!Spawn button properly integrated into waiting_approval/verified header with dynamic test IDs based on status.
276-286: LGTM!Spawn Sub-Task action correctly added to the in-progress dropdown menu, maintaining consistency with other status states.
apps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsx (9)
86-89: LGTM!The spawn mode props are well-typed with sensible defaults. The optional nature allows backward compatibility with existing callers.
104-107: LGTM!Clean derivation of
isSpawnModefrom the presence ofparentFeature.
135-138: LGTM!Using
Set<string>forselectedAncestorIdsprovides efficient membership checks. State initialization is appropriate.
167-190: LGTM!The ancestor initialization logic correctly computes and pre-selects all ancestors when spawning. Default selection of all ancestors is good UX.
Note: Ensure
allFeatureshas a stable reference (e.g., from store state) to avoid unnecessary effect re-runs if this array is recreated on each render.
215-237: LGTM!The description composition logic correctly:
- Creates a parent context object for uniform handling
- Combines with computed ancestors
- Only prepends context when selections exist
- Uses a clear markdown structure with separators
253-255: LGTM!Correctly adds only the parent as a dependency in spawn mode. The ancestor selections provide context (prepended to description) while the parent establishes the actual dependency relationship.
353-358: LGTM!The dialog header provides clear context about the spawn operation, including a preview of the parent task. The truncation at 50 characters keeps the dialog description readable.
378-392: LGTM!The
AncestorContextSectionis correctly conditionally rendered and receives properly shaped props. Extracting only the needed properties fromparentFeatureis a clean approach.
586-586: LGTM!Button text appropriately reflects the action context.
Shironex
added
Enhancement
- Add edge dragging to create dependencies in graph view - Add spawn sub-task action available in graph view and kanban board - Implement ancestor context selection when spawning tasks - Add dependency validation (circular, self, duplicate prevention) - Include ancestor context in spawned task descriptions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Consolidated dependency validation and ancestor traversal utilities: - wouldCreateCircularDependency, dependencyExists -> @automaker/dependency-resolver - getAncestors, formatAncestorContextForPrompt, AncestorContext -> @automaker/dependency-resolver - Removed graph-view/utils directory (now redundant) - Updated all imports to use shared package 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
JBotwina
force-pushed
the
JBotwina/task-deps-spawn
branch
from
8c1322d to
76b7cfe
Compare
…nhancements - Added onDeleteTask functionality to allow task deletion from both board and graph views. - Integrated delete options for dependencies in the graph view, enhancing user interaction. - Updated ancestor context section to clarify the role of parent tasks in task descriptions. - Improved layout handling in graph view to preserve node positions during updates. This update enhances task management capabilities and improves user experience in the graph view.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/ui/src/components/views/graph-view/graph-view.tsx (1)
85-130: Dependencies may not persist to backend - verify integration.A previous review flagged that
onUpdateFeatureis bound tohandleUpdateFeatureinuse-board-actions.ts, which has a narrowed type signature that may excludedependencies. Ensure the handler properly accepts and persists thedependenciesfield, or this feature won't work as expected.#!/bin/bash # Verify handleUpdateFeature accepts dependencies in its update shape rg -nP "handleUpdateFeature" apps/ui/src/components/views/board-view/hooks/use-board-actions.ts -A 20 | head -50apps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsx (1)
352-357: Consider using title with fallback instead of truncated description.The dialog subtitle truncates the description mid-word with
parentFeature?.description.slice(0, 50). A previous review suggested usingparentFeature.titleas the primary display with description as fallback.🔎 Proposed improvement
- ? `Create a sub-task that depends on "${parentFeature?.title || parentFeature?.description.slice(0, 50)}..."` + ? `Create a sub-task that depends on "${parentFeature?.title || (parentFeature?.description?.slice(0, 50) + '...')}"`Or better, use a utility function that truncates at word boundaries.
🧹 Nitpick comments (6)
apps/ui/src/components/views/graph-view/graph-canvas.tsx (1)
131-151: Consider consolidating the duplicate position preservation logic.Lines 131-139 and 143-149 contain identical logic. While separating them documents the different intents, you could consolidate:
🔎 Optional refactor to reduce duplication
- } else if (hasNewNodes) { - // New nodes added - need to re-layout but try to preserve existing positions - setNodes((currentNodes) => { - const positionMap = new Map(currentNodes.map((n) => [n.id, n.position])); - return layoutedNodes.map((node) => ({ - ...node, - position: positionMap.get(node.id) || node.position, - })); - }); - setEdges(layoutedEdges); - } else { - // No new nodes - just update data without changing positions + } else { + // Preserve existing positions, use layout position for new nodes setNodes((currentNodes) => { const positionMap = new Map(currentNodes.map((n) => [n.id, n.position])); return layoutedNodes.map((node) => ({ ...node, position: positionMap.get(node.id) || node.position, })); }); - // Update edges without triggering re-render of nodes setEdges(layoutedEdges); }libs/dependency-resolver/src/resolver.ts (1)
287-323: Consider clarifying depth semantics in documentation.The
depthvalue represents the traversal depth starting from 0 for immediate parents. However,formatAncestorContextForPromptusesdepth: -1as a special marker for the parent task (see lines 341-343). This convention mismatch could be confusing sincegetAncestorsnever returnsdepth: -1.Consider documenting that
depth: -1is a sentinel value only used when manually constructingAncestorContextfor parent tasks outside ofgetAncestors.apps/ui/src/components/views/graph-view/graph-view.tsx (1)
119-119: Unnecessary type assertion ondependencies.The cast
as string[] | undefinedis redundant if theFeaturetype already definesdependenciesasstring[]. If the type is correct upstream, this cast can be removed.🔎 Proposed simplification
- const currentDeps = (targetFeature.dependencies as string[] | undefined) || []; + const currentDeps = targetFeature.dependencies ?? [];apps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsx (1)
167-177: Ancestors fetched for parent but parent itself isn't an ancestor.The
getAncestors(parentFeature, allFeatures)call retrieves ancestors of the parent feature (i.e., grandparents and beyond), not the parent itself. Later inhandleAdd(lines 218-231), aparentContextis manually constructed withdepth: -1and merged with ancestors. This is correct behavior, but the state variable nameancestorscould be clearer (e.g.,parentAncestorsorancestorsOfParent).apps/ui/src/components/ui/collapsible.tsx (1)
1-1: Optional: Remove unused React import.The React import is not used in this file since there's no JSX or React API usage. Consider removing it for clarity.
🔎 Proposed fix
-import * as React from 'react'; import * as CollapsiblePrimitive from '@radix-ui/react-collapsible';apps/ui/src/components/views/graph-view/hooks/use-graph-layout.ts (1)
115-125: Optional: remove unnecessary dependency.The
getLayoutedElementscallback has an empty dependency array (line 71), making it stable across renders. Including it in therunLayoutdependency array (line 124) is unnecessary, though harmless.🔎 Proposed cleanup
[nodes, edges, getLayoutedElements, setNodes, fitView] + // getLayoutedElements has empty deps and is stable, can be omitted + [nodes, edges, setNodes, fitView] );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
apps/ui/package.jsonapps/ui/src/components/ui/collapsible.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/components/kanban-card/card-header.tsxapps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsxapps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsxapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/board-view/kanban-board.tsxapps/ui/src/components/views/board-view/shared/ancestor-context-section.tsxapps/ui/src/components/views/board-view/shared/index.tsapps/ui/src/components/views/graph-view/components/dependency-edge.tsxapps/ui/src/components/views/graph-view/components/task-node.tsxapps/ui/src/components/views/graph-view/graph-canvas.tsxapps/ui/src/components/views/graph-view/graph-view.tsxapps/ui/src/components/views/graph-view/hooks/use-graph-layout.tsapps/ui/src/components/views/graph-view/hooks/use-graph-nodes.tslibs/dependency-resolver/src/index.tslibs/dependency-resolver/src/resolver.tslibs/dependency-resolver/tests/resolver.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/ui/package.json
- apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx
- apps/ui/src/components/views/board-view/shared/ancestor-context-section.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
libs/dependency-resolver/src/resolver.ts (1)
libs/dependency-resolver/src/index.ts (5)
wouldCreateCircularDependency(10-10)dependencyExists(11-11)AncestorContext(15-15)getAncestors(12-12)formatAncestorContextForPrompt(13-13)
libs/dependency-resolver/tests/resolver.test.ts (1)
libs/dependency-resolver/src/resolver.ts (2)
wouldCreateCircularDependency(223-250)dependencyExists(260-264)
apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx (1)
apps/ui/src/components/ui/button.tsx (1)
Button(108-108)
apps/ui/src/components/views/graph-view/components/dependency-edge.tsx (2)
apps/ui/src/store/app-store.ts (1)
Feature(256-272)apps/ui/src/lib/utils.ts (1)
cn(5-7)
🔇 Additional comments (35)
apps/ui/src/components/views/board-view/shared/index.ts (1)
9-9: LGTM!The new re-export follows the existing pattern and correctly exposes the
AncestorContextSectioncomponent through the shared module's public API.apps/ui/src/components/views/board-view/hooks/use-board-actions.ts (3)
101-101: LGTM!Clean addition of the optional
dependenciesfield to the feature creation parameter, enabling dependency tracking from the point of feature creation.
148-155: LGTM!Good defensive approach providing an empty array default for
dependencieswhen not specified, ensuring the created feature always has a valid array for the dependencies field.
371-386: Soft blocking with toast warning is a good UX choice.The implementation allows starting features with incomplete dependencies while informing the user. This provides flexibility without being overly restrictive.
apps/ui/src/components/views/graph-view/components/dependency-edge.tsx (2)
82-91: Good UX pattern with the invisible hover path.The wider transparent stroke (20px) makes edge hover detection much more user-friendly, especially for thin edges that would otherwise be difficult to target.
128-157: LGTM!The delete button implementation is well-structured:
- Appears on hover or selection, providing clear affordance
- Uses
EdgeLabelRenderercorrectly for proper positioning- Maintains hover state when interacting with the button itself (lines 138-139)
- Proper z-index ensures visibility
apps/ui/src/components/views/board-view/kanban-board.tsx (2)
44-44: LGTM!The optional
onSpawnTaskcallback follows the established pattern of other action callbacks inKanbanBoardProps.
189-189: LGTM!Correctly wraps the callback to pass the feature context while using optional chaining to safely handle the undefined case.
libs/dependency-resolver/tests/resolver.test.ts (2)
354-496: Excellent test coverage forwouldCreateCircularDependency.The tests comprehensively cover:
- Base cases (no deps, valid chains)
- Direct and transitive cycles
- Complex patterns (diamond, parallel branches)
- Edge cases (self-dependency, missing features, empty list)
The parallel branches test (lines 413-426) is particularly valuable to ensure no false positives.
498-552: Good coverage fordependencyExists.The test at lines 518-528 correctly validates that this function only checks direct dependencies, not transitive ones—important for clarity on the function's contract.
apps/ui/src/components/views/graph-view/components/task-node.tsx (2)
94-105: LGTM!Adding explicit
id="target"andisConnectable={true}enables proper edge connection targeting. The styling with hover feedback provides good visual affordance.
273-294: Good safety guard on Delete Task.The
!data.isRunningcondition (line 283) prevents accidental deletion of actively running tasks, which could cause issues with orphaned processes.apps/ui/src/components/views/board-view.tsx (4)
110-111: LGTM!Clean state introduction for tracking the parent feature during spawn mode.
1027-1030: LGTM!The spawn task handler correctly sets the parent feature context before opening the dialog, enabling the
AddFeatureDialogto pre-configure the new task with the dependency relationship.
1093-1098: Current implementation is correct.Clearing
spawnParentFeatureon dialog close is sufficient. When the dialog opens via the normal "Add Feature" flow (line 937),spawnParentFeatureis either alreadynullor was cleared from a previous close. The past review suggestion to add an else branch is unnecessary complexity.
1053-1058: LGTM!Good integration of both
onSpawnTaskandonDeleteTaskhandlers for the GraphView, completing the feature parity between Kanban and Graph views for task management actions.apps/ui/src/components/views/graph-view/graph-canvas.tsx (2)
113-156: Good layout preservation strategy.The three-branch approach (initial render, new nodes, no new nodes) correctly handles:
- First render: apply full layout
- New nodes added: layout new nodes while preserving user-positioned existing nodes
- Data updates only: preserve all positions
The
positionMappattern elegantly handles the fallback to layout position for new nodes.
184-204: LGTM!The connection handling is well-structured:
handleConnectdelegates toonCreateDependencyfor validation and persistenceisValidConnectionprovides fast client-side rejection of self-connections- Validation (circular deps, duplicates) correctly happens in the upstream handler
libs/dependency-resolver/src/resolver.ts (3)
223-250: LGTM! Well-structured circular dependency detection.The DFS-based reachability check correctly identifies cycles by checking if
sourceIdcan reachtargetIdthrough existing dependencies before adding the new edge. Thevisitedset properly prevents infinite loops in cyclic graphs.
260-264: LGTM! Simple and correct dependency existence check.
334-396: LGTM! Clear markdown formatting with appropriate separation of parent and ancestor contexts.The function correctly handles the special
depth: -1case for parent tasks with emphasized formatting, and gracefully handles empty selections by returning an empty string.libs/dependency-resolver/src/index.ts (1)
6-16: LGTM! Clean barrel export expansion.All new utilities and the
AncestorContexttype are properly re-exported, maintaining a clean public API surface.apps/ui/src/components/views/graph-view/graph-view.tsx (1)
165-190: LGTM! Callback implementations follow existing patterns.The new
onSpawnTask,onDeleteTask, andonDeleteDependencycallbacks are correctly implemented with consistent feature lookup and optional chaining patterns.apps/ui/src/components/views/graph-view/hooks/use-graph-nodes.ts (2)
27-28: LGTM! Interface extensions are consistent and well-typed.The new optional callbacks (
onSpawnTask,onDeleteTask,onDeleteDependency) follow the established pattern of the existing callbacks and are properly typed.Also applies to: 46-48
121-126: LGTM! Callback binding follows established patterns.The new callbacks are bound correctly using the same pattern as existing callbacks (checking existence before binding).
Also applies to: 156-156
apps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsx (3)
214-236: LGTM! Context construction logic is correct.The parent is correctly added as a special
AncestorContextwithdepth: -1, merged with ancestors, and formatted. The conditional check ensures context is only prepended when ancestors are selected.
252-253: LGTM! Parent automatically added as dependency in spawn mode.This ensures spawned tasks correctly depend on their parent, which aligns with the PR objectives.
377-391: LGTM! AncestorContextSection properly wired.The component receives all necessary props for displaying and selecting ancestor context.
apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx (2)
35-35: LGTM! New callback prop properly typed and destructured.The optional
onSpawnTaskcallback follows the same pattern as existing callbacks likeonViewOutput.Also applies to: 45-45
98-108: LGTM! Spawn Sub-Task action consistently implemented across all card states.The spawn action is:
- Present in running, backlog, waiting_approval/verified, and in_progress states
- Uses
GitForkicon consistently- Includes proper
e.stopPropagation()handling- Has appropriate
data-testidattributes for testingAlso applies to: 123-137, 172-187, 276-286
apps/ui/src/components/ui/collapsible.tsx (1)
2-2: The @radix-ui/react-collapsible dependency is current and secure.The package is at version 1.1.12, which is the latest available, and there are no known security vulnerabilities.
apps/ui/src/components/views/graph-view/hooks/use-graph-layout.ts (4)
1-1: LGTM!The addition of
useRefto the imports is appropriate for the position caching functionality introduced in this file.
55-60: LGTM!The position caching logic correctly computes node positions from dagre output and stores them in the cache for reuse.
85-112: The conditional layout logic is correctly implemented.The caching optimization properly distinguishes between structural changes (nodes added/removed) and non-structural changes. When the structure is unchanged, cached positions are reused, avoiding expensive dagre recalculations.
Note: The edge-change re-layout concern has been flagged in a previous comment on lines 74-82.
74-82: Confirm design intent before deciding on edge-change re-layout behavior.The current implementation intentionally excludes edges from
structureKeyto avoid expensive layout recalculations on every edge change—this is documented in the code comment (line 75). However, when users drag edges to create dependencies, the graph doesn't automatically reorganize to reflect the new hierarchy; they must manually click a layout button to trigger re-layout.This creates a suboptimal workflow: create dependency → manually invoke layout. The control panel buttons (Horizontal/Vertical Layout) do provide a way to re-layout, and they are visible with tooltips, but the workflow isn't seamless.
Before changing this design, clarify:
- Is the current manual re-layout workflow acceptable for the expected user behavior?
- If users expect automatic re-layout on dependency creation, consider one of these options:
- Include edge structure in
structureKeyto auto-trigger re-layout (performance cost)- Auto-trigger
runLayoutafter successful dependency creation (debounced to batch multiple operations)- Improve discoverability of the layout buttons via more prominent UI or automatic triggering on edge changes
The code is working as designed—verify whether the design itself meets product requirements.
| // Cache the last computed positions to avoid recalculating layout | ||
| const positionCache = useRef<Map<string, { x: number; y: number }>>(new Map()); | ||
| const lastStructureKey = useRef<string>(''); |
There was a problem hiding this comment.
Potential memory leak: stale entries accumulate in positionCache.
When nodes are removed from the graph, their cached positions remain in positionCache indefinitely. The cache is only cleared when nodes.length === 0 (line 87), but removed nodes' entries are never pruned. Over time, if nodes are frequently added and removed, the cache will grow unbounded.
🔎 Proposed fix to clean up stale cache entries
After line 23, add a cleanup effect or prune stale entries when processing the layout. For example, add this after computing layoutedElements:
}, [nodes, edges, structureKey, getLayoutedElements]);
+
+ // Clean up stale cache entries
+ useMemo(() => {
+ const currentNodeIds = new Set(nodes.map(n => n.id));
+ for (const cachedId of positionCache.current.keys()) {
+ if (!currentNodeIds.has(cachedId)) {
+ positionCache.current.delete(cachedId);
+ }
+ }
+ }, [nodes]);Alternatively, prune inside the layoutedElements useMemo logic when structure changes (around line 97).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
apps/ui/src/components/views/graph-view/hooks/use-graph-layout.ts lines 21-23:
positionCache (a Map held in a ref) accumulates entries for nodes that have been
removed; prune stale entries whenever the node set changes by removing map keys
not present in the current nodes list (or clear the map when nodes.length ===
0). Implement this by adding a small cleanup: on nodes change (useEffect with
nodes as dependency) iterate over positionCache.current keys and delete any key
not found in the current nodes' ids (or clear entirely if nodes is empty);
alternatively perform the same pruning at the start of the layoutedElements
useMemo so the cache never grows with removed node ids. Ensure you mutate
positionCache.current (delete keys) rather than reassigning the ref.
There was a problem hiding this comment.
@JBotwina can you check if this is a real issue and fix in another pr?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
3 participants
Summary
Adds task dependency management and sub-task spawning to the graph and kanban views.
Features
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.