Skip to content

Commit 082ea7e

Browse files
committed
🤖 fix: address review feedback for subagents implementation
- Fix processQueue parallel limit enforcement - re-check running count each iteration instead of once before the loop - Add args field to tool-call-end event schema so TaskService can receive agent_report arguments for report delivery - Fix config→metadata passing parentWorkspaceId/agentType/taskState fields so UI nesting works correctly - Deduplicate sortWithNesting function - export from workspaceFiltering.ts and import in useSortedWorkspacesByProject.ts (DRY violation fix) - Add error handling to TasksSection.tsx - properly catch API errors instead of leaving UI stuck in 'Loading...' state - Extract magic numbers for indentation to named constants (BASE_PADDING_PX, NESTING_INDENT_PX) - Update tests and mock types to include new args field in tool-call-end events Change-Id: I7fcc69e8616f758a68b5864ebe6afb318dafbb85 Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 parent 89461c1 commit 082ea7e

File tree

13 files changed

+83
-73
lines changed

13 files changed

+83
-73
lines changed

src/browser/components/Settings/sections/TasksSection.tsx

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,19 +18,28 @@ export function TasksSection() {
1818
const { api } = useAPI();
1919
const [settings, setSettings] = useState<TaskSettings>(DEFAULT_TASK_SETTINGS);
2020
const [loaded, setLoaded] = useState(false);
21+
const [loadError, setLoadError] = useState<string | null>(null);
2122

2223
// Load settings on mount
2324
useEffect(() => {
2425
if (api) {
25-
void api.general.getTaskSettings().then((taskSettings) => {
26-
setSettings({
27-
maxParallelAgentTasks:
28-
taskSettings.maxParallelAgentTasks ?? DEFAULT_TASK_SETTINGS.maxParallelAgentTasks,
29-
maxTaskNestingDepth:
30-
taskSettings.maxTaskNestingDepth ?? DEFAULT_TASK_SETTINGS.maxTaskNestingDepth,
26+
api.general
27+
.getTaskSettings()
28+
.then((taskSettings) => {
29+
setSettings({
30+
maxParallelAgentTasks:
31+
taskSettings.maxParallelAgentTasks ?? DEFAULT_TASK_SETTINGS.maxParallelAgentTasks,
32+
maxTaskNestingDepth:
33+
taskSettings.maxTaskNestingDepth ?? DEFAULT_TASK_SETTINGS.maxTaskNestingDepth,
34+
});
35+
setLoaded(true);
36+
})
37+
.catch((error) => {
38+
console.error("Failed to load task settings:", error);
39+
setLoadError("Failed to load settings");
40+
// Still mark as loaded so user can interact with defaults
41+
setLoaded(true);
3142
});
32-
setLoaded(true);
33-
});
3443
}
3544
}, [api]);
3645

@@ -82,6 +91,9 @@ export function TasksSection() {
8291
<p className="text-muted mb-4 text-xs">
8392
Configure limits for agent subworkspaces spawned via the task tool.
8493
</p>
94+
{loadError && (
95+
<p className="mb-4 text-xs text-red-500">{loadError}. Using default values.</p>
96+
)}
8597

8698
<div className="space-y-4">
8799
<div className="flex items-center justify-between">

src/browser/components/WorkspaceListItem.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,12 @@ const WorkspaceListItemInner: React.FC<WorkspaceListItemProps> = ({
105105
const { canInterrupt, awaitingUserQuestion } = useWorkspaceSidebarState(workspaceId);
106106
const isWorking = canInterrupt && !awaitingUserQuestion;
107107

108-
// Calculate left padding based on nesting depth (base 9px + 16px per level)
109-
const leftPadding = 9 + nestingDepth * 16;
108+
// Sidebar indentation constants
109+
const BASE_PADDING_PX = 9;
110+
const NESTING_INDENT_PX = 16;
111+
112+
// Calculate left padding based on nesting depth
113+
const leftPadding = BASE_PADDING_PX + nestingDepth * NESTING_INDENT_PX;
110114
const isAgentTask = Boolean(metadata.parentWorkspaceId);
111115

112116
return (

src/browser/hooks/useSortedWorkspacesByProject.ts

Lines changed: 3 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -3,65 +3,10 @@ import { useWorkspaceContext } from "@/browser/contexts/WorkspaceContext";
33
import type { FrontendWorkspaceMetadata } from "@/common/types/workspace";
44
import { useWorkspaceRecency } from "@/browser/stores/WorkspaceStore";
55
import { useStableReference, compareMaps } from "@/browser/hooks/useStableReference";
6+
import { sortWithNesting, type WorkspaceWithNesting } from "@/browser/utils/ui/workspaceFiltering";
67

7-
/** Workspace metadata extended with computed nesting depth */
8-
export interface WorkspaceWithNesting extends FrontendWorkspaceMetadata {
9-
/** Nesting depth (0 = top-level, 1 = direct child, etc.) */
10-
nestingDepth: number;
11-
}
12-
13-
/**
14-
* Sort workspaces so children appear immediately after their parent.
15-
* Maintains recency order within each level.
16-
*/
17-
function sortWithNesting(
18-
metadataList: FrontendWorkspaceMetadata[],
19-
workspaceRecency: Record<string, number>
20-
): WorkspaceWithNesting[] {
21-
// Build parent→children map
22-
const childrenByParent = new Map<string, FrontendWorkspaceMetadata[]>();
23-
const topLevel: FrontendWorkspaceMetadata[] = [];
24-
25-
for (const ws of metadataList) {
26-
const parentId = ws.parentWorkspaceId;
27-
if (parentId) {
28-
const siblings = childrenByParent.get(parentId) ?? [];
29-
siblings.push(ws);
30-
childrenByParent.set(parentId, siblings);
31-
} else {
32-
topLevel.push(ws);
33-
}
34-
}
35-
36-
// Sort by recency (most recent first)
37-
const sortByRecency = (a: FrontendWorkspaceMetadata, b: FrontendWorkspaceMetadata) => {
38-
const aTs = workspaceRecency[a.id] ?? 0;
39-
const bTs = workspaceRecency[b.id] ?? 0;
40-
return bTs - aTs;
41-
};
42-
43-
topLevel.sort(sortByRecency);
44-
for (const children of childrenByParent.values()) {
45-
children.sort(sortByRecency);
46-
}
47-
48-
// Flatten: parent, then children recursively
49-
const result: WorkspaceWithNesting[] = [];
50-
51-
const visit = (ws: FrontendWorkspaceMetadata, depth: number) => {
52-
result.push({ ...ws, nestingDepth: depth });
53-
const children = childrenByParent.get(ws.id) ?? [];
54-
for (const child of children) {
55-
visit(child, depth + 1);
56-
}
57-
};
58-
59-
for (const ws of topLevel) {
60-
visit(ws, 0);
61-
}
62-
63-
return result;
64-
}
8+
// Re-export for backward compatibility
9+
export type { WorkspaceWithNesting };
6510

6611
export function useSortedWorkspacesByProject() {
6712
const { projects } = useProjectContext();

src/browser/utils/messages/StreamingMessageAggregator.status.test.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
130130
// Complete the tool call
131131
aggregator.handleToolCallEnd({
132132
type: "tool-call-end",
133+
args: {},
133134
workspaceId: "workspace1",
134135
messageId,
135136
toolCallId,
@@ -171,6 +172,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
171172

172173
aggregator.handleToolCallEnd({
173174
type: "tool-call-end",
175+
args: {},
174176
workspaceId: "workspace1",
175177
messageId,
176178
toolCallId: "tool1",
@@ -195,6 +197,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
195197

196198
aggregator.handleToolCallEnd({
197199
type: "tool-call-end",
200+
args: {},
198201
workspaceId: "workspace1",
199202
messageId,
200203
toolCallId: "tool2",
@@ -234,6 +237,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
234237

235238
aggregator.handleToolCallEnd({
236239
type: "tool-call-end",
240+
args: {},
237241
workspaceId: "workspace1",
238242
messageId,
239243
toolCallId: "tool1",
@@ -286,6 +290,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
286290
// Complete with failure
287291
aggregator.handleToolCallEnd({
288292
type: "tool-call-end",
293+
args: {},
289294
workspaceId: "workspace1",
290295
messageId,
291296
toolCallId: "tool1",
@@ -323,6 +328,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
323328

324329
aggregator.handleToolCallEnd({
325330
type: "tool-call-end",
331+
args: {},
326332
workspaceId: "workspace1",
327333
messageId: "msg1",
328334
toolCallId: "tool1",
@@ -387,6 +393,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
387393
// Complete with validation failure
388394
aggregator.handleToolCallEnd({
389395
type: "tool-call-end",
396+
args: {},
390397
workspaceId: "workspace1",
391398
messageId,
392399
toolCallId: "tool1",
@@ -446,6 +453,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
446453
// Complete successfully
447454
aggregator.handleToolCallEnd({
448455
type: "tool-call-end",
456+
args: {},
449457
workspaceId: "workspace1",
450458
messageId,
451459
toolCallId: "tool1",
@@ -691,6 +699,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
691699

692700
aggregator.handleToolCallEnd({
693701
type: "tool-call-end",
702+
args: {},
694703
workspaceId: "workspace1",
695704
messageId,
696705
toolCallId,
@@ -735,6 +744,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
735744
// Complete the tool call
736745
aggregator.handleToolCallEnd({
737746
type: "tool-call-end",
747+
args: {},
738748
workspaceId: "workspace1",
739749
messageId,
740750
toolCallId,
@@ -778,6 +788,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
778788

779789
aggregator.handleToolCallEnd({
780790
type: "tool-call-end",
791+
args: {},
781792
workspaceId: "workspace1",
782793
messageId,
783794
toolCallId: "tool1",
@@ -802,6 +813,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
802813

803814
aggregator.handleToolCallEnd({
804815
type: "tool-call-end",
816+
args: {},
805817
workspaceId: "workspace1",
806818
messageId,
807819
toolCallId: "tool2",
@@ -830,6 +842,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
830842

831843
aggregator.handleToolCallEnd({
832844
type: "tool-call-end",
845+
args: {},
833846
workspaceId: "workspace1",
834847
messageId,
835848
toolCallId: "tool3",
@@ -872,6 +885,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
872885

873886
aggregator.handleToolCallEnd({
874887
type: "tool-call-end",
888+
args: {},
875889
workspaceId: "workspace1",
876890
messageId: messageId1,
877891
toolCallId: "tool1",
@@ -918,6 +932,7 @@ describe("StreamingMessageAggregator - Agent Status", () => {
918932

919933
aggregator.handleToolCallEnd({
920934
type: "tool-call-end",
935+
args: {},
921936
workspaceId: "workspace1",
922937
messageId: messageId2,
923938
toolCallId: "tool2",

src/browser/utils/messages/StreamingMessageAggregator.test.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,7 @@ describe("StreamingMessageAggregator", () => {
170170

171171
aggregator.handleToolCallEnd({
172172
type: "tool-call-end",
173+
args: {},
173174
workspaceId: "test-workspace",
174175
messageId: "msg1",
175176
toolCallId: "tool1",
@@ -226,6 +227,7 @@ describe("StreamingMessageAggregator", () => {
226227

227228
aggregator.handleToolCallEnd({
228229
type: "tool-call-end",
230+
args: {},
229231
workspaceId: "test-workspace",
230232
messageId: "msg1",
231233
toolCallId: "tool1",
@@ -360,6 +362,7 @@ describe("StreamingMessageAggregator", () => {
360362

361363
aggregator.handleToolCallEnd({
362364
type: "tool-call-end",
365+
args: {},
363366
workspaceId: "test-workspace",
364367
messageId: "msg1",
365368
toolCallId: "tool1",

src/browser/utils/ui/workspaceFiltering.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ export interface WorkspaceWithNesting extends FrontendWorkspaceMetadata {
2020
* Sort workspaces so children appear immediately after their parent.
2121
* Maintains recency order within each level.
2222
*/
23-
function sortWithNesting(
23+
export function sortWithNesting(
2424
metadataList: FrontendWorkspaceMetadata[],
2525
workspaceRecency: Record<string, number>
2626
): WorkspaceWithNesting[] {

src/common/orpc/schemas/stream.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,7 @@ export const ToolCallEndEventSchema = z.object({
168168
messageId: z.string(),
169169
toolCallId: z.string(),
170170
toolName: z.string(),
171+
args: z.unknown().meta({ description: "Tool input arguments (for listeners that need args)" }),
171172
result: z.unknown(),
172173
timestamp: z.number().meta({ description: "When tool call completed (Date.now())" }),
173174
});

src/node/config.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,10 @@ export class Config {
308308
// GUARANTEE: All workspaces must have runtimeConfig (apply default if missing)
309309
runtimeConfig: workspace.runtimeConfig ?? DEFAULT_RUNTIME_CONFIG,
310310
aiSettings: workspace.aiSettings,
311+
// Agent task workspace fields (optional - only set for subagent workspaces)
312+
parentWorkspaceId: workspace.parentWorkspaceId,
313+
agentType: workspace.agentType,
314+
taskState: workspace.taskState,
311315
};
312316

313317
// Migrate missing createdAt to config for next load

src/node/services/mock/mockScenarioPlayer.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,7 @@ export class MockScenarioPlayer {
314314
messageId,
315315
toolCallId: event.toolCallId,
316316
toolName: event.toolName,
317+
args: event.args,
317318
result: event.result,
318319
timestamp: Date.now(),
319320
};

src/node/services/mock/scenarioTypes.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ export interface MockToolEndEvent extends MockAssistantEventBase {
6060
kind: "tool-end";
6161
toolCallId: string;
6262
toolName: string;
63+
args?: unknown;
6364
result: unknown;
6465
}
6566

0 commit comments

Comments
 (0)