feat(review): add business context (task/requirement) to diff viewer file tree (#480)#522
Conversation
WalkthroughThis change adds task/requirement context to the Review page by extending types, updating FileTreePanel and DiffViewer to accept and render task metadata and grouping, fetching tasks in the Review page, and adding tests and icon mocks to support the new UI behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant ReviewPage as ReviewPage
participant TasksAPI as TasksAPI
participant FileTreePanel as FileTreePanel
participant DiffViewer as DiffViewer
ReviewPage->>TasksAPI: GET /api/v2/tasks?workspace_path=...
TasksAPI-->>ReviewPage: task list
ReviewPage->>ReviewPage: derive contextTask (filter IN_PROGRESS)
ReviewPage->>FileTreePanel: props (files, tasks, contextTask)
ReviewPage->>DiffViewer: props (diffFiles, changedFiles, tasks, contextTask)
FileTreePanel->>FileTreePanel: group files by dir or task, resolve titles/badges
DiffViewer->>DiffViewer: map changedFiles -> task, render requirement ID, task title, View Task link
FileTreePanel-->>ReviewPage: (UI interactions: group toggle, collapse)
DiffViewer-->>ReviewPage: (UI interactions: file select)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
PR #522 Code Review: Business Context (Task/Requirement) in Diff ViewerOverall this is a well-scoped, cleanly implemented feature. The graceful-degradation design is a strong choice, and the test coverage is solid. I have a few issues worth addressing before merge, ranging from a functional bug to some architecture considerations. Critical / Functional1. In Recommend verifying what 2. In Important3. The type comment in The backend This is acceptable as forward-looking scaffolding, but it should be documented more explicitly (e.g. a TODO comment linking to the issue that will add backend enrichment), or the acceptance criteria updated to reflect that the per-file task badge currently only works via 4. In { onError: () => {} }This intentionally swallows errors, which is good for the "graceful degradation" goal. However, it also suppresses network failures, 5xx responses, and auth errors without any logging. If an engineer later needs to debug why task context is missing, there will be no signal. Consider at minimum 5.
Accessibility6. "View Task →" arrow is a raw Unicode character with no accessible label In <Link href="/tasks" ...>View Task →</Link>The 7. Dir/Task toggle buttons lack an explicit active/pressed state for assistive technology The grouping toggle buttons in Test Coverage8. No test covering the There is a test for "renders task chip via per-file changedFiles mapping" but no test for the case where 9. The Minor / Nice-to-have
Summary
The most important issues to address before merge are #1 (potential key mismatch for renamed files) and #3 (clarifying that per-file mapping is forward-scaffolding with no backend source today). Everything else is quality hardening that can be tracked or addressed as follow-up. Reviewed files:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
web-ui/src/components/review/FileTreePanel.tsx (1)
180-184: Consider clarifying badge display behavior for mixed-task scenarios.When
contextTaskis set, files without explicittask_titlewill display the context task's title as a badge. This could be misleading if a file was actually modified by a different task (withtask_idset but notask_title).Consider only showing the badge when the file's attribution is explicitly known:
♻️ Optional refinement
- {(file.task_title ?? contextTask?.title) && ( + {file.task_title && ( <span className="mx-1 shrink-0 rounded bg-muted px-1 py-0.5 text-[10px] text-muted-foreground"> - {file.task_title ?? contextTask?.title} + {file.task_title} </span> )}Alternatively, if the intent is to show
contextTaskfor untagged files, consider showing it only when!file.task_idto avoid conflicts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/review/FileTreePanel.tsx` around lines 180 - 184, The badge currently renders whenever file.task_title or contextTask?.title exists which can misattribute files that have a task_id but no task_title; update the render condition so the badge shows file.task_title when present, otherwise show contextTask?.title only if the file has no task_id (i.e., use file.task_title ?? (!file.task_id ? contextTask?.title : undefined)); apply this change where the JSX uses (file.task_title ?? contextTask?.title) and keep the same span markup but guard against showing the context task for files that already have a task_id.web-ui/__tests__/components/review/FileTreePanel.test.tsx (1)
67-67: Minor inconsistency in button query selectors.Lines 67 and 98 query the same "Task" toggle button but use different patterns (
/task/ivs/group by task/i). Consider using a consistent selector for clarity:♻️ Optional refinement for consistency
- await user.click(screen.getByRole('button', { name: /task/i })); + await user.click(screen.getByRole('button', { name: /group by task/i }));Both work due to how
getByRolematches againstaria-labeland text content, but consistency improves test maintainability.Also applies to: 98-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/__tests__/components/review/FileTreePanel.test.tsx` at line 67, The test uses two different button name regexes for the same "Task" toggle; make them consistent by replacing both occurrences of await user.click(screen.getByRole('button', { name: /task/i })); and await user.click(screen.getByRole('button', { name: /group by task/i })); with the same selector (choose one, e.g., await user.click(screen.getByRole('button', { name: /group by task/i }));) in FileTreePanel.test.tsx so both user.click calls target the identical getByRole('button', { name: /group by task/i }) matcher for clarity and maintainability.web-ui/src/components/review/DiffViewer.tsx (1)
191-197: Consider linking to the specific task detail page.The "View Task" link currently navigates to the generic
/taskspage. SincefileTaskhas anidproperty, you could provide a more direct navigation experience:♻️ Optional enhancement
<Link - href="/tasks" + href={`/tasks?highlight=${fileTask.id}`} className="text-primary hover:underline" onClick={(e) => e.stopPropagation()} > View Task → </Link>Or if the execution page supports task-specific views:
- href="/tasks" + href={`/execution/${fileTask.id}`}This would align better with the PR objective to "navigate to task detail" without requiring the user to find the task manually.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/review/DiffViewer.tsx` around lines 191 - 197, The current Link component in DiffViewer renders a generic href "/tasks"; update it to navigate to the specific task detail using the fileTask id (e.g., set Link's href to the route that includes fileTask.id such as `/tasks/${fileTask.id}` or the app's task-detail route), keeping the existing onClick={(e) => e.stopPropagation()} behavior; locate the Link in DiffViewer.tsx and replace the static "/tasks" href with the task-specific path that uses fileTask.id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/__tests__/components/review/FileTreePanel.test.tsx`:
- Line 67: The test uses two different button name regexes for the same "Task"
toggle; make them consistent by replacing both occurrences of await
user.click(screen.getByRole('button', { name: /task/i })); and await
user.click(screen.getByRole('button', { name: /group by task/i })); with the
same selector (choose one, e.g., await user.click(screen.getByRole('button', {
name: /group by task/i }));) in FileTreePanel.test.tsx so both user.click calls
target the identical getByRole('button', { name: /group by task/i }) matcher for
clarity and maintainability.
In `@web-ui/src/components/review/DiffViewer.tsx`:
- Around line 191-197: The current Link component in DiffViewer renders a
generic href "/tasks"; update it to navigate to the specific task detail using
the fileTask id (e.g., set Link's href to the route that includes fileTask.id
such as `/tasks/${fileTask.id}` or the app's task-detail route), keeping the
existing onClick={(e) => e.stopPropagation()} behavior; locate the Link in
DiffViewer.tsx and replace the static "/tasks" href with the task-specific path
that uses fileTask.id.
In `@web-ui/src/components/review/FileTreePanel.tsx`:
- Around line 180-184: The badge currently renders whenever file.task_title or
contextTask?.title exists which can misattribute files that have a task_id but
no task_title; update the render condition so the badge shows file.task_title
when present, otherwise show contextTask?.title only if the file has no task_id
(i.e., use file.task_title ?? (!file.task_id ? contextTask?.title : undefined));
apply this change where the JSX uses (file.task_title ?? contextTask?.title) and
keep the same span markup but guard against showing the context task for files
that already have a task_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d9224c1-c852-4781-a79e-0c5de6807655
📒 Files selected for processing (7)
web-ui/__mocks__/@hugeicons/react.jsweb-ui/__tests__/components/review/DiffViewer.test.tsxweb-ui/__tests__/components/review/FileTreePanel.test.tsxweb-ui/src/app/review/page.tsxweb-ui/src/components/review/DiffViewer.tsxweb-ui/src/components/review/FileTreePanel.tsxweb-ui/src/types/index.ts
|
test comment to verify access |
|
Review of PR 522 - Dir/Task grouping feature |
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 `@web-ui/src/components/review/FileTreePanel.tsx`:
- Around line 57-75: grouping currently computes a group title on first insert
using different fallbacks per mode which causes mislabeling; change
groupedByTask to compute a single deterministic resolved title per file before
inserting: for each file derive taskId = file.task_id ?? contextTask?.id ??
'unassigned' and resolve taskTitle = file.task_title ??
taskTitleById.get(file.task_id) ?? (file.task_id ? undefined :
contextTask?.title) ?? 'Unassigned' (i.e. prefer embedded title, then tasks
lookup by file.task_id, then contextTask only when file has no task_id), then
use that resolved title when creating or merging into groups and if merging into
an existing group update its title when the existing title is a weaker
placeholder (like 'Unassigned') so later files with real titles correct the
header; update references to taskTitleById, groups, files, tasks, contextTask,
file.task_id, and file.task_title accordingly.
- Line 218: The span rendering the task marker in FileTreePanel.tsx currently
hard-codes the color class "bg-amber-500"; replace that hard-coded palette with
the Nova/Shadcn semantic token or variant used across the app (e.g., the
project's Nova token such as "bg-primary" or the appropriate shadcn variant
class) so the marker follows the configured gray theme; update the span with the
semantic class and ensure any helper/utility (e.g., cn usage) is applied
consistently instead of the literal "bg-amber-500".
🪄 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: cdb45244-1981-4019-ac44-42b1db03ab64
📒 Files selected for processing (1)
web-ui/src/components/review/FileTreePanel.tsx
| const groupedByTask = useMemo(() => { | ||
| const taskTitleById = new Map(tasks?.map((t) => [t.id, t.title]) ?? []); | ||
| const groups = new Map<string, { title: string; files: FileChange[] }>(); | ||
| for (const file of files) { | ||
| const taskId = file.task_id ?? contextTask?.id ?? 'unassigned'; | ||
| const taskTitle = | ||
| file.task_title ?? | ||
| taskTitleById.get(taskId) ?? | ||
| contextTask?.title ?? | ||
| 'Unassigned'; | ||
| const existing = groups.get(taskId); | ||
| if (existing) { | ||
| existing.files.push(file); | ||
| } else { | ||
| groups.set(taskId, { title: taskTitle, files: [file] }); | ||
| } | ||
| } | ||
| return groups; | ||
| }, [files, tasks, contextTask]); |
There was a problem hiding this comment.
Unify task-title resolution; current fallbacks can mislabel or hide the task context.
In task mode, a file with an explicit task_id can still inherit contextTask?.title when its title is missing, so task B can render under task A’s name. In dir mode, the opposite happens: files with task_id but no embedded task_title never consult tasks, so the badge disappears even when the title is available. Because the group title is fixed on first insert, a later file with the real title also will not correct the header.
Suggested fix
+ const taskTitleById = useMemo(
+ () => new Map(tasks?.map((task) => [task.id, task.title]) ?? []),
+ [tasks]
+ );
+
const groupedByTask = useMemo(() => {
- const taskTitleById = new Map(tasks?.map((t) => [t.id, t.title]) ?? []);
const groups = new Map<string, { title: string; files: FileChange[] }>();
for (const file of files) {
const taskId = file.task_id ?? contextTask?.id ?? 'unassigned';
const taskTitle =
file.task_title ??
- taskTitleById.get(taskId) ??
- contextTask?.title ??
+ (file.task_id ? taskTitleById.get(file.task_id) : contextTask?.title) ??
'Unassigned';
const existing = groups.get(taskId);
if (existing) {
+ if (existing.title === 'Unassigned' && taskTitle !== 'Unassigned') {
+ existing.title = taskTitle;
+ }
existing.files.push(file);
} else {
groups.set(taskId, { title: taskTitle, files: [file] });
}
}
return groups;
- }, [files, tasks, contextTask]);
+ }, [files, taskTitleById, contextTask]);
@@
{dirFiles.map((file) => {
const Icon = changeTypeIcon[file.change_type];
const iconColor = changeTypeColor[file.change_type];
const isSelected = selectedFile === file.path;
+ const taskTitle =
+ file.task_title ??
+ (file.task_id ? taskTitleById.get(file.task_id) : contextTask?.title);
return (
<button
@@
- {(file.task_title ?? (!file.task_id ? contextTask?.title : undefined)) && (
+ {taskTitle && (
<span className="mx-1 shrink-0 rounded bg-muted px-1 py-0.5 text-[10px] text-muted-foreground">
- {file.task_title ?? contextTask?.title}
+ {taskTitle}
</span>
)}Also applies to: 180-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/review/FileTreePanel.tsx` around lines 57 - 75,
grouping currently computes a group title on first insert using different
fallbacks per mode which causes mislabeling; change groupedByTask to compute a
single deterministic resolved title per file before inserting: for each file
derive taskId = file.task_id ?? contextTask?.id ?? 'unassigned' and resolve
taskTitle = file.task_title ?? taskTitleById.get(file.task_id) ?? (file.task_id
? undefined : contextTask?.title) ?? 'Unassigned' (i.e. prefer embedded title,
then tasks lookup by file.task_id, then contextTask only when file has no
task_id), then use that resolved title when creating or merging into groups and
if merging into an existing group update its title when the existing title is a
weaker placeholder (like 'Unassigned') so later files with real titles correct
the header; update references to taskTitleById, groups, files, tasks,
contextTask, file.task_id, and file.task_title accordingly.
| ) : ( | ||
| <ArrowDown01Icon className="h-3 w-3 shrink-0" /> | ||
| )} | ||
| <span className="h-2 w-2 rounded-full bg-amber-500" /> |
There was a problem hiding this comment.
Use a theme token instead of bg-amber-500 for the task marker.
This hard-codes a non-gray palette color into the tree and bypasses the Nova theme tokens. Prefer a semantic token or Shadcn variant so it stays aligned with the configured color scheme.
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/review/FileTreePanel.tsx` at line 218, The span
rendering the task marker in FileTreePanel.tsx currently hard-codes the color
class "bg-amber-500"; replace that hard-coded palette with the Nova/Shadcn
semantic token or variant used across the app (e.g., the project's Nova token
such as "bg-primary" or the appropriate shadcn variant class) so the marker
follows the configured gray theme; update the span with the semantic class and
ensure any helper/utility (e.g., cn usage) is applied consistently instead of
the literal "bg-amber-500".
|
(See the full formatted review in the comment above.) |
|
(test cleanup) |
ReviewGood feature work. The Dir/Task grouping toggle, per-file task badges, and graceful degradation pattern are solid design choices. CodeRabbit covered the key nitpicks. A few additional observations: Bug: Shared collapsedDirs state across view modes Bug: contextTask badge misattribution (echoing CodeRabbit) UX: View Task link target (echoing CodeRabbit) Silent error swallowing Backend readiness gap Tests — Coverage is thorough across both components. No gaps beyond the selector inconsistency CodeRabbit flagged. Summary: The collapsed state bleed and badge misattribution bugs plus the /tasks link precision are worth addressing before merge. Silent error handler and TODO comment are lower priority. Overall a clean implementation. |
1 participant
Summary
Closes #480
FileChangetype with optionaltask_id?andtask_title?fields (backward-compatible)IN_PROGRESStask ascontextTaskFileTreePanel: Dir/Task grouping toggle, per-file task badges in dir mode, collapsible task groups in task mode —contextTaskused as fallback when files lack explicittask_idDiffViewer: Per-file task context chip + "View Task →" link in each file header (resolves viachangedFilesmapping, falls back tocontextTask, gracefully absent when neither is set)Acceptance Criteria
REQ: {id}chip)/tasks)changedFilesmapping)Test Plan
cd web-ui && npm test -- --watchAll=false --testPathPattern=review→ 18 tests passnpm run lint→ 0 errorsSummary by CodeRabbit
New Features
Tests