[Amber] Fix: [Amber] Todo/TodoWrite Visualization#848
[Amber] Fix: [Amber] Todo/TodoWrite Visualization#848github-actions[bot] wants to merge 1 commit intomainfrom
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change adds TodoWrite-specific visualization support to the tool-message component. It introduces a TodoItem type and parsing logic, implements a TodoListView component to render tasks with status indicators and priority badges, and updates tool summary generation to display formatted task counts and statuses. The tool header now displays a ListTodo icon for TodoWrite tools. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/frontend/src/components/ui/tool-message.tsx`:
- Around line 38-43: The parseTodoItems function unsafely casts input.todos to
TodoItem[]; update parseTodoItems to validate each array element has the
required fields (e.g., content as string and status as expected enum/string)
before returning; either filter out invalid entries or return null if any
element is malformed. Reference parseTodoItems and ensure the output shape
matches what TodoListView expects (accessing todo.content and todo.status) by
performing a type guard check on each item and returning a correctly typed
TodoItem[] or null.
- Around line 53-61: The rendering logic for todo.status (checked in the
component that uses todo.status and icons Check and Loader2) lacks a fallback
for unexpected values; update the JSX to include a default branch that renders a
sensible fallback icon or placeholder (e.g., a neutral icon or aria-hidden empty
circle) when todo.status is not "completed", "in_progress", or "pending", and
ensure it provides appropriate accessibility attributes (aria-label or title) so
unexpected statuses still display meaningfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa501764-3a24-4b2e-918e-ee152959e6d2
📒 Files selected for processing (1)
components/frontend/src/components/ui/tool-message.tsx
| const parseTodoItems = (input?: Record<string, unknown>): TodoItem[] | null => { | ||
| if (!input) return null; | ||
| const todos = input.todos; | ||
| if (!Array.isArray(todos) || todos.length === 0) return null; | ||
| return todos as TodoItem[]; | ||
| }; |
There was a problem hiding this comment.
Unsafe type assertion may cause runtime errors with malformed data.
parseTodoItems casts the array directly to TodoItem[] without validating that each element has the required content and status properties. If the backend sends incomplete data, accessing todo.status or todo.content in TodoListView will silently fail or render incorrectly.
🛡️ Proposed fix to add validation
const parseTodoItems = (input?: Record<string, unknown>): TodoItem[] | null => {
if (!input) return null;
const todos = input.todos;
if (!Array.isArray(todos) || todos.length === 0) return null;
- return todos as TodoItem[];
+ return todos
+ .filter(
+ (t): t is TodoItem =>
+ t != null &&
+ typeof t === "object" &&
+ typeof (t as Record<string, unknown>).content === "string" &&
+ ["pending", "in_progress", "completed"].includes(
+ (t as Record<string, unknown>).status as string
+ )
+ );
};📝 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.
| const parseTodoItems = (input?: Record<string, unknown>): TodoItem[] | null => { | |
| if (!input) return null; | |
| const todos = input.todos; | |
| if (!Array.isArray(todos) || todos.length === 0) return null; | |
| return todos as TodoItem[]; | |
| }; | |
| const parseTodoItems = (input?: Record<string, unknown>): TodoItem[] | null => { | |
| if (!input) return null; | |
| const todos = input.todos; | |
| if (!Array.isArray(todos) || todos.length === 0) return null; | |
| return todos | |
| .filter( | |
| (t): t is TodoItem => | |
| t != null && | |
| typeof t === "object" && | |
| typeof (t as Record<string, unknown>).content === "string" && | |
| ["pending", "in_progress", "completed"].includes( | |
| (t as Record<string, unknown>).status as string | |
| ) | |
| ); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/components/ui/tool-message.tsx` around lines 38 - 43,
The parseTodoItems function unsafely casts input.todos to TodoItem[]; update
parseTodoItems to validate each array element has the required fields (e.g.,
content as string and status as expected enum/string) before returning; either
filter out invalid entries or return null if any element is malformed. Reference
parseTodoItems and ensure the output shape matches what TodoListView expects
(accessing todo.content and todo.status) by performing a type guard check on
each item and returning a correctly typed TodoItem[] or null.
| {todo.status === "completed" && ( | ||
| <Check className="w-3.5 h-3.5 text-green-500" /> | ||
| )} | ||
| {todo.status === "in_progress" && ( | ||
| <Loader2 className="w-3.5 h-3.5 text-blue-500 animate-spin" /> | ||
| )} | ||
| {todo.status === "pending" && ( | ||
| <div className="w-3.5 h-3.5 rounded-full border-2 border-muted-foreground/40" /> | ||
| )} |
There was a problem hiding this comment.
Missing fallback for unexpected status values.
If todo.status contains an unexpected value (not one of the three defined statuses), no icon will be rendered. Consider adding a fallback for robustness.
🛡️ Proposed fix to add fallback
{todo.status === "pending" && (
<div className="w-3.5 h-3.5 rounded-full border-2 border-muted-foreground/40" />
)}
+ {!["completed", "in_progress", "pending"].includes(todo.status) && (
+ <div className="w-3.5 h-3.5 rounded-full border-2 border-muted-foreground/40" />
+ )}Note: If the validation fix in parseTodoItems is applied, this becomes redundant.
📝 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.
| {todo.status === "completed" && ( | |
| <Check className="w-3.5 h-3.5 text-green-500" /> | |
| )} | |
| {todo.status === "in_progress" && ( | |
| <Loader2 className="w-3.5 h-3.5 text-blue-500 animate-spin" /> | |
| )} | |
| {todo.status === "pending" && ( | |
| <div className="w-3.5 h-3.5 rounded-full border-2 border-muted-foreground/40" /> | |
| )} | |
| {todo.status === "completed" && ( | |
| <Check className="w-3.5 h-3.5 text-green-500" /> | |
| )} | |
| {todo.status === "in_progress" && ( | |
| <Loader2 className="w-3.5 h-3.5 text-blue-500 animate-spin" /> | |
| )} | |
| {todo.status === "pending" && ( | |
| <div className="w-3.5 h-3.5 rounded-full border-2 border-muted-foreground/40" /> | |
| )} | |
| {!["completed", "in_progress", "pending"].includes(todo.status) && ( | |
| <div className="w-3.5 h-3.5 rounded-full border-2 border-muted-foreground/40" /> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/components/ui/tool-message.tsx` around lines 53 - 61,
The rendering logic for todo.status (checked in the component that uses
todo.status and icons Check and Loader2) lacks a fallback for unexpected values;
update the JSX to include a default branch that renders a sensible fallback icon
or placeholder (e.g., a neutral icon or aria-hidden empty circle) when
todo.status is not "completed", "in_progress", or "pending", and ensure it
provides appropriate accessibility attributes (aria-label or title) so
unexpected statuses still display meaningfully.
|
This should be closed and re-opened with clean requirements including being feature flagged off to start. |
Review Queue Status
Action needed: Resolve merge conflicts (rebase on main)
|
Renders TodoWrite tool calls as a structured task list with status icons (completed/in_progress/pending), priority badges, and a smart summary showing task counts by status in the collapsed view. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ambient-code
bot
force-pushed
the
amber/issue-847-amber-todo-todowrite-visualization
branch
from
468ac5c to
30feeb0
Compare
PR Fixer: Issues Resolved
🤖 Session |
2 participants
Automated Fix by Amber Agent
This PR addresses issue #847 using the Amber background agent.
Changes Summary
Fixes Applied (PR Fixer Session)
tool-message.tsx(kept both TodoWrite and AskUserQuestion summaries)parseTodoItems— now validates each element has requiredcontentandstatusproperties (CodeRabbit review)TodoListView(CodeRabbit review)Pre-merge Checklist
Reviewer Notes
This PR was automatically generated. Please review:
🤖 Generated with Amber Background Agent
Closes #847