-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: implement hierarchical task execution history with HTA-inspired visualization #7527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… visualization - Add parentId field to HistoryItem type for parent-child relationships - Update Task class to track and persist parent task relationships - Create useTaskHierarchy hook with tree building and flattening utilities - Update HistoryView with tree/flat view toggle and expand/collapse controls - Enhance TaskItem with hierarchical display including indentation and chevrons - Add comprehensive tests for hierarchical data structure utilities - Add translation keys for new UI elements This implementation allows users to visualize task relationships in a tree structure, making it easier to understand complex orchestrated workflows and trace task lineage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this code and even I'm confused by the import order.
| } | ||
|
|
||
| // Import useState | ||
| import { useState } from "react" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional? The useState import is at the bottom of the file (line 142) instead of at the top with other React imports. This breaks the standard import organization pattern. Consider moving it to line 1:
| import { useState } from "react" | |
| import { useMemo, useState } from "react" | |
| import type { HistoryItem } from "@roo-code/types" |
| onDelete={setDeleteTaskId} | ||
| className="m-2" | ||
| isHierarchical={showHierarchical} | ||
| level={showHierarchical ? (item as any).level || 0 : 0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid using any type casting here? Lines 279-282 use (item as any).level and similar assertions which bypass TypeScript's type safety. Consider properly typing displayTasks as HierarchicalHistoryItem[] when showHierarchical is true.
| tasks.forEach((task) => { | ||
| const hierarchicalTask = taskMap.get(task.id)! | ||
|
|
||
| if (task.parentId && taskMap.has(task.parentId)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if a parentId references a non-existent task? The current implementation doesn't handle this edge case, which could lead to orphaned tasks being silently dropped. Consider adding a fallback to treat orphaned children as root tasks:
| if (task.parentId && taskMap.has(task.parentId)) { | |
| if (task.parentId && taskMap.has(task.parentId)) { | |
| // This is a child task | |
| const parent = taskMap.get(task.parentId)! | |
| parent.children.push(hierarchicalTask) | |
| hierarchicalTask.level = parent.level + 1 | |
| } else { | |
| // This is a root task (or orphaned child) | |
| rootTasks.push(hierarchicalTask) | |
| } |
| const [showHierarchical, setShowHierarchical] = useState(true) | ||
|
|
||
| // Use the hierarchical task hook | ||
| const { flattenedTasks, expandedIds, toggleExpanded, expandAll, collapseAll } = useTaskHierarchy(tasks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance consideration: The flattenedTasks computation happens on every render when showHierarchical changes. Since useTaskHierarchy already uses useMemo internally, this might be acceptable, but have you considered the performance impact with large task lists?
| }, | ||
| "viewAllHistory": "View all tasks" | ||
| "viewAllHistory": "View all tasks", | ||
| "expandAll": "Expand All", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice we're only updating the English locale file. Should we add placeholder translations or TODO comments in other language files to ensure translators know about these new keys? Missing translations could cause UI issues for non-English users.
Summary
This PR implements hierarchical task execution history with HTA-inspired visualization as requested in #7526. The implementation provides a tree-like structure for viewing task relationships, making it easier to understand complex orchestrated workflows.
Changes
Core Implementation
parentIdfield toHistoryItemtype for tracking parent-child relationshipsTaskclass to persist parent task relationships throughtaskMetadatauseTaskHierarchyhook with tree building and flattening utilitiesUI Enhancements
TaskItemcomponent with:Testing & Quality
Screenshots
The new hierarchical view provides:
Review Results
Code review completed with 95% confidence score and PROCEED recommendation:
Testing
pnpm testto execute all testsCloses #7526
Important
Implement hierarchical task execution history with HTA-inspired visualization, enhancing task relationship understanding through a tree-like structure.
parentIdtoHistoryItemtype inhistory.tsfor parent-child tracking.Taskclass inTask.tsto persist parent task relationships.useTaskHierarchyhook inuseTaskHierarchy.tsfor tree building and flattening.HistoryView.tsx.TaskItem.tsx.TaskItemwith visual indentation and chevron icons.useTaskHierarchy.spec.ts.history.jsonfor new UI elements.This description was created by
for b733d5e. You can customize this summary. It will automatically update as commits are pushed.