Skip to content

Commit 068ac5e

Browse files
committed
Address code review feedback for Tasks panel IPC
- Remove unused `scope` feature from IPC protocol (YAGNI) - Remove redundant array spreads in TasksPanel - Make TaskDetails extend TaskActions to reduce duplication - Remove handler fallback in TasksPanel (keep requests/commands separate) - Add notification subscription support to useIpc hook - Add tests for onNotification feature - Remove redundant individual exports from tasks/api.ts (export as group only)
1 parent abdf3f3 commit 068ac5e

File tree

13 files changed

+506
-418
lines changed

13 files changed

+506
-418
lines changed

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -537,7 +537,7 @@
537537
"extensionPack": [
538538
"ms-vscode-remote.remote-ssh"
539539
],
540-
"packageManager": "pnpm@10.27.0",
540+
"packageManager": "pnpm@10.28.2",
541541
"engines": {
542542
"vscode": "^1.95.0",
543543
"node": ">= 20"

packages/shared/src/ipc/protocol.ts

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,23 +10,20 @@
1010
/** Request definition: params P, response R */
1111
export interface RequestDef<P = void, R = void> {
1212
readonly method: string;
13-
readonly scope?: string;
1413
/** @internal Phantom types for inference - not present at runtime */
1514
readonly _types?: { params: P; response: R };
1615
}
1716

1817
/** Command definition: params P, no response */
1918
export interface CommandDef<P = void> {
2019
readonly method: string;
21-
readonly scope?: string;
2220
/** @internal Phantom type for inference - not present at runtime */
2321
readonly _types?: { params: P };
2422
}
2523

2624
/** Notification definition: data D (extension to webview) */
2725
export interface NotificationDef<D = void> {
2826
readonly method: string;
29-
readonly scope?: string;
3027
/** @internal Phantom type for inference - not present at runtime */
3128
readonly _types?: { data: D };
3229
}
@@ -36,25 +33,20 @@ export interface NotificationDef<D = void> {
3633
/** Define a request with typed params and response */
3734
export function defineRequest<P = void, R = void>(
3835
method: string,
39-
scope?: string,
4036
): RequestDef<P, R> {
41-
return { method, scope } as RequestDef<P, R>;
37+
return { method } as RequestDef<P, R>;
4238
}
4339

4440
/** Define a fire-and-forget command */
45-
export function defineCommand<P = void>(
46-
method: string,
47-
scope?: string,
48-
): CommandDef<P> {
49-
return { method, scope } as CommandDef<P>;
41+
export function defineCommand<P = void>(method: string): CommandDef<P> {
42+
return { method } as CommandDef<P>;
5043
}
5144

5245
/** Define a push notification (extension to webview) */
5346
export function defineNotification<D = void>(
5447
method: string,
55-
scope?: string,
5648
): NotificationDef<D> {
57-
return { method, scope } as NotificationDef<D>;
49+
return { method } as NotificationDef<D>;
5850
}
5951

6052
// --- Wire format ---
@@ -63,7 +55,6 @@ export function defineNotification<D = void>(
6355
export interface IpcRequest<P = unknown> {
6456
readonly requestId: string;
6557
readonly method: string;
66-
readonly scope?: string;
6758
readonly params?: P;
6859
}
6960

packages/shared/src/tasks/api.ts

Lines changed: 18 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -17,20 +17,18 @@ import {
1717

1818
import type { Task, TaskDetails, TaskLogEntry, TaskTemplate } from "./types";
1919

20-
// --- Requests ---
21-
2220
export interface InitResponse {
2321
tasks: Task[];
2422
templates: TaskTemplate[];
2523
baseUrl: string;
2624
tasksSupported: boolean;
2725
}
2826

29-
export const init = defineRequest<void, InitResponse>("init");
30-
export const getTasks = defineRequest<void, Task[]>("getTasks");
31-
export const getTemplates = defineRequest<void, TaskTemplate[]>("getTemplates");
32-
export const getTask = defineRequest<{ taskId: string }, Task>("getTask");
33-
export const getTaskDetails = defineRequest<{ taskId: string }, TaskDetails>(
27+
const init = defineRequest<void, InitResponse>("init");
28+
const getTasks = defineRequest<void, Task[]>("getTasks");
29+
const getTemplates = defineRequest<void, TaskTemplate[]>("getTemplates");
30+
const getTask = defineRequest<{ taskId: string }, Task>("getTask");
31+
const getTaskDetails = defineRequest<{ taskId: string }, TaskDetails>(
3432
"getTaskDetails",
3533
);
3634

@@ -39,31 +37,25 @@ export interface CreateTaskParams {
3937
prompt: string;
4038
presetId?: string;
4139
}
42-
export const createTask = defineRequest<CreateTaskParams, Task>("createTask");
43-
44-
export const deleteTask = defineRequest<{ taskId: string }, void>("deleteTask");
45-
export const pauseTask = defineRequest<{ taskId: string }, void>("pauseTask");
46-
export const resumeTask = defineRequest<{ taskId: string }, void>("resumeTask");
40+
const createTask = defineRequest<CreateTaskParams, Task>("createTask");
4741

48-
// --- Commands ---
42+
const deleteTask = defineRequest<{ taskId: string }, void>("deleteTask");
43+
const pauseTask = defineRequest<{ taskId: string }, void>("pauseTask");
44+
const resumeTask = defineRequest<{ taskId: string }, void>("resumeTask");
4945

50-
export const viewInCoder = defineCommand<{ taskId: string }>("viewInCoder");
51-
export const viewLogs = defineCommand<{ taskId: string }>("viewLogs");
52-
export const downloadLogs = defineCommand<{ taskId: string }>("downloadLogs");
53-
export const sendTaskMessage = defineCommand<{
46+
const viewInCoder = defineCommand<{ taskId: string }>("viewInCoder");
47+
const viewLogs = defineCommand<{ taskId: string }>("viewLogs");
48+
const downloadLogs = defineCommand<{ taskId: string }>("downloadLogs");
49+
const sendTaskMessage = defineCommand<{
5450
taskId: string;
5551
message: string;
5652
}>("sendTaskMessage");
5753

58-
// --- Notifications ---
59-
60-
export const taskUpdated = defineNotification<Task>("taskUpdated");
61-
export const tasksUpdated = defineNotification<Task[]>("tasksUpdated");
62-
export const logsAppend = defineNotification<TaskLogEntry[]>("logsAppend");
63-
export const refresh = defineNotification<void>("refresh");
64-
export const showCreateForm = defineNotification<void>("showCreateForm");
65-
66-
// --- Grouped export ---
54+
const taskUpdated = defineNotification<Task>("taskUpdated");
55+
const tasksUpdated = defineNotification<Task[]>("tasksUpdated");
56+
const logsAppend = defineNotification<TaskLogEntry[]>("logsAppend");
57+
const refresh = defineNotification<void>("refresh");
58+
const showCreateForm = defineNotification<void>("showCreateForm");
6759

6860
export const TasksApi = {
6961
// Requests

packages/shared/src/tasks/types.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,10 @@ export type LogsStatus = "ok" | "not_available" | "error";
4444
/**
4545
* Full details for a selected task, including logs and action availability.
4646
*/
47-
export interface TaskDetails {
47+
export interface TaskDetails extends TaskActions {
4848
task: Task;
4949
logs: TaskLogEntry[];
5050
logsStatus: LogsStatus;
51-
canResume: boolean;
52-
canPause: boolean;
5351
}
5452

5553
export interface TaskActions {

packages/tasks/src/hooks/useTasksApi.ts

Lines changed: 17 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -9,44 +9,32 @@
99
* ```
1010
*/
1111

12-
import {
13-
createTask as createTaskDef,
14-
deleteTask as deleteTaskDef,
15-
downloadLogs as downloadLogsDef,
16-
getTask as getTaskDef,
17-
getTaskDetails as getTaskDetailsDef,
18-
getTasks as getTasksDef,
19-
getTemplates as getTemplatesDef,
20-
init as initDef,
21-
pauseTask as pauseTaskDef,
22-
resumeTask as resumeTaskDef,
23-
sendTaskMessage as sendTaskMessageDef,
24-
viewInCoder as viewInCoderDef,
25-
viewLogs as viewLogsDef,
26-
type CreateTaskParams,
27-
} from "@repo/shared";
12+
import { TasksApi, type CreateTaskParams } from "@repo/shared";
2813
import { useIpc } from "@repo/webview-shared/react";
2914

3015
export function useTasksApi() {
3116
const { request, command } = useIpc();
3217

3318
return {
3419
// Requests
35-
init: () => request(initDef),
36-
getTasks: () => request(getTasksDef),
37-
getTemplates: () => request(getTemplatesDef),
38-
getTask: (taskId: string) => request(getTaskDef, { taskId }),
39-
getTaskDetails: (taskId: string) => request(getTaskDetailsDef, { taskId }),
40-
createTask: (params: CreateTaskParams) => request(createTaskDef, params),
41-
deleteTask: (taskId: string) => request(deleteTaskDef, { taskId }),
42-
pauseTask: (taskId: string) => request(pauseTaskDef, { taskId }),
43-
resumeTask: (taskId: string) => request(resumeTaskDef, { taskId }),
20+
init: () => request(TasksApi.init),
21+
getTasks: () => request(TasksApi.getTasks),
22+
getTemplates: () => request(TasksApi.getTemplates),
23+
getTask: (taskId: string) => request(TasksApi.getTask, { taskId }),
24+
getTaskDetails: (taskId: string) =>
25+
request(TasksApi.getTaskDetails, { taskId }),
26+
createTask: (params: CreateTaskParams) =>
27+
request(TasksApi.createTask, params),
28+
deleteTask: (taskId: string) => request(TasksApi.deleteTask, { taskId }),
29+
pauseTask: (taskId: string) => request(TasksApi.pauseTask, { taskId }),
30+
resumeTask: (taskId: string) => request(TasksApi.resumeTask, { taskId }),
4431

4532
// Commands
46-
viewInCoder: (taskId: string) => command(viewInCoderDef, { taskId }),
47-
viewLogs: (taskId: string) => command(viewLogsDef, { taskId }),
48-
downloadLogs: (taskId: string) => command(downloadLogsDef, { taskId }),
33+
viewInCoder: (taskId: string) => command(TasksApi.viewInCoder, { taskId }),
34+
viewLogs: (taskId: string) => command(TasksApi.viewLogs, { taskId }),
35+
downloadLogs: (taskId: string) =>
36+
command(TasksApi.downloadLogs, { taskId }),
4937
sendTaskMessage: (taskId: string, message: string) =>
50-
command(sendTaskMessageDef, { taskId, message }),
38+
command(TasksApi.sendTaskMessage, { taskId, message }),
5139
};
5240
}

packages/webview-shared/src/index.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ export interface WebviewMessage {
33
method: string;
44
params?: unknown;
55
requestId?: string;
6-
scope?: string;
76
}
87

98
// VS Code state API

packages/webview-shared/src/react/useIpc.ts

Lines changed: 80 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { useEffect, useRef } from "react";
77

88
import { postMessage } from "../api";
99

10-
import type { IpcResponse } from "@repo/shared";
10+
import type { IpcNotification, IpcResponse } from "@repo/shared";
1111

1212
interface PendingRequest {
1313
resolve: (value: unknown) => void;
@@ -20,10 +20,10 @@ const DEFAULT_TIMEOUT_MS = 30000;
2020
export interface UseIpcOptions {
2121
/** Request timeout in ms (default: 30000) */
2222
timeoutMs?: number;
23-
/** Scope for message routing */
24-
scope?: string;
2523
}
2624

25+
type NotificationHandler = (data: unknown) => void;
26+
2727
/**
2828
* Hook for type-safe IPC with the extension.
2929
*
@@ -32,11 +32,21 @@ export interface UseIpcOptions {
3232
* const ipc = useIpc();
3333
* const tasks = await ipc.request(getTasks); // Type: Task[]
3434
* ipc.command(viewInCoder, { taskId: "123" }); // Fire-and-forget
35+
*
36+
* // Subscribe to notifications
37+
* useEffect(() => {
38+
* return ipc.onNotification(tasksUpdated, (tasks) => {
39+
* setTasks(tasks); // tasks is typed as Task[]
40+
* });
41+
* }, []);
3542
* ```
3643
*/
3744
export function useIpc(options: UseIpcOptions = {}) {
38-
const { timeoutMs = DEFAULT_TIMEOUT_MS, scope } = options;
45+
const { timeoutMs = DEFAULT_TIMEOUT_MS } = options;
3946
const pendingRequests = useRef<Map<string, PendingRequest>>(new Map());
47+
const notificationHandlers = useRef<Map<string, Set<NotificationHandler>>>(
48+
new Map(),
49+
);
4050

4151
// Cleanup on unmount
4252
useEffect(() => {
@@ -46,27 +56,43 @@ export function useIpc(options: UseIpcOptions = {}) {
4656
req.reject(new Error("Component unmounted"));
4757
}
4858
pendingRequests.current.clear();
59+
notificationHandlers.current.clear();
4960
};
5061
}, []);
5162

52-
// Handle responses
63+
// Handle responses and notifications
5364
useEffect(() => {
5465
const handler = (event: MessageEvent) => {
55-
const msg = event.data as IpcResponse | undefined;
56-
if (!msg || typeof msg.requestId !== "string" || !("success" in msg)) {
66+
const msg = event.data as IpcResponse | IpcNotification | undefined;
67+
68+
if (!msg || typeof msg !== "object") {
5769
return;
5870
}
5971

60-
const pending = pendingRequests.current.get(msg.requestId);
61-
if (!pending) return;
72+
// Response handling (has requestId + success)
73+
if ("requestId" in msg && "success" in msg) {
74+
const pending = pendingRequests.current.get(msg.requestId);
75+
if (!pending) return;
76+
77+
clearTimeout(pending.timeout);
78+
pendingRequests.current.delete(msg.requestId);
6279

63-
clearTimeout(pending.timeout);
64-
pendingRequests.current.delete(msg.requestId);
80+
if (msg.success) {
81+
pending.resolve(msg.data);
82+
} else {
83+
pending.reject(new Error(msg.error || "Request failed"));
84+
}
85+
return;
86+
}
6587

66-
if (msg.success) {
67-
pending.resolve(msg.data);
68-
} else {
69-
pending.reject(new Error(msg.error || "Request failed"));
88+
// Notification handling (has type, no requestId)
89+
if ("type" in msg && !("requestId" in msg)) {
90+
const handlers = notificationHandlers.current.get(msg.type);
91+
if (handlers) {
92+
for (const h of handlers) {
93+
h(msg.data);
94+
}
95+
}
7096
}
7197
};
7298

@@ -78,7 +104,6 @@ export function useIpc(options: UseIpcOptions = {}) {
78104
function request<P, R>(
79105
definition: {
80106
method: string;
81-
scope?: string;
82107
_types?: { params: P; response: R };
83108
},
84109
...args: P extends void ? [] : [params: P]
@@ -102,7 +127,6 @@ export function useIpc(options: UseIpcOptions = {}) {
102127

103128
postMessage({
104129
method: definition.method,
105-
scope: scope ?? definition.scope,
106130
requestId,
107131
params,
108132
});
@@ -111,15 +135,51 @@ export function useIpc(options: UseIpcOptions = {}) {
111135

112136
/** Send command without waiting (fire-and-forget) */
113137
function command<P>(
114-
definition: { method: string; scope?: string; _types?: { params: P } },
138+
definition: { method: string; _types?: { params: P } },
115139
...args: P extends void ? [] : [params: P]
116140
): void {
117141
postMessage({
118142
method: definition.method,
119-
scope: scope ?? definition.scope,
120143
params: args[0],
121144
});
122145
}
123146

124-
return { request, command };
147+
/**
148+
* Subscribe to push notifications from the extension.
149+
* Returns an unsubscribe function that should be called on cleanup.
150+
*
151+
* @example
152+
* ```tsx
153+
* useEffect(() => {
154+
* return ipc.onNotification(tasksUpdated, (tasks) => {
155+
* setTasks(tasks);
156+
* });
157+
* }, []);
158+
* ```
159+
*/
160+
function onNotification<D>(
161+
definition: { method: string; _types?: { data: D } },
162+
callback: (data: D) => void,
163+
): () => void {
164+
const method = definition.method;
165+
let handlers = notificationHandlers.current.get(method);
166+
if (!handlers) {
167+
handlers = new Set();
168+
notificationHandlers.current.set(method, handlers);
169+
}
170+
handlers.add(callback as NotificationHandler);
171+
172+
// Return unsubscribe function
173+
return () => {
174+
const h = notificationHandlers.current.get(method);
175+
if (h) {
176+
h.delete(callback as NotificationHandler);
177+
if (h.size === 0) {
178+
notificationHandlers.current.delete(method);
179+
}
180+
}
181+
};
182+
}
183+
184+
return { request, command, onNotification };
125185
}

0 commit comments

Comments
 (0)