Skip to content

Commit e34e10a

Browse files
authored
fix(web-ui): deliver batch notifications when execution monitor is unmounted (#652) (#691)
* fix(web-ui): deliver batch notifications when execution monitor is unmounted (#652) Async batch.completed / blocker.created notifications previously only fired while BatchExecutionMonitor was mounted (i.e. on /execution?batch=...). Navigating away meant a background batch finished silently — defeating the purpose of browser notifications. Add a cross-page background watcher (useBatchNotificationWatcher) mounted once inside NotificationProvider (which lives in the root layout, so it runs on every route). It polls the existing GET /api/v2/batches endpoint, establishes a baseline on first poll so already-terminal/blocked batches don't notify, and fires batch.completed on terminal transitions and blocker.created on per-task BLOCKED transitions. An in-flight guard prevents overlapping slow polls from corrupting the transition baseline. Make it the single dispatcher: remove the duplicate dispatch from BatchExecutionMonitor. No backend change — the batches list endpoint already returns per-task results. * docs: update notification known-limitation note for #652 background delivery * fix(web-ui): ignore stale poll results after workspace switch (#652 review)
1 parent 305726b commit e34e10a

7 files changed

Lines changed: 463 additions & 57 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ Issue **import + traceability** (#565) is **complete**: `POST /api/v2/integratio
4545
**Phase 5.4 is complete** — PRD stress-test web UI: trigger + streaming (#561). Backend: `GET /api/v2/prd/stress-test` SSE endpoint streams `goals_extracted`, `goal_analyzed`, `complete`, and `error` events from `core/prd_stress_test.py:stress_test_prd_stream()`, resolving the LLM provider via the standard chain and applying the standard rate limit. Frontend: `useStressTestStream` hook manages the SSE connection and event accumulation; `StressTestModal` renders the streaming progress and is opened via a "Stress Test" button on the `/prd` page (enabled only when a PRD exists). Results rendering + refinement (#562) is **complete**: the `complete` SSE event now carries structured, severity-tagged `ambiguities` (`Ambiguity.severity` is `"blocking"`/`"warning"`); `StressTestModal` shows a results view of `AmbiguityCard`s (question text, severity badge, answer textarea) with an "X of Y answered" progress indicator and a **[Refine PRD]** button (disabled until every blocking ambiguity is answered). Refine posts to `POST /api/v2/prd/stress-test/refine`, which folds the answers into a new PRD version via `resolve_ambiguities_into_prd` (offloaded with `asyncio.to_thread`) and `prd.create_new_version`, then `mutatePrd` reflects it in the editor.
4646

4747
**Phase 5.3 is complete** — Async notifications cover both surfaces:
48-
- **Browser + in-app center (#559)**: `useNotifications` hook with workspace-scoped `localStorage` persistence and browser Notification dispatch (only when tab hidden + permission granted); `NotificationProvider` in root layout; `NotificationCenter` (bell icon + dropdown) mounts in sidebar footer. `BatchExecutionMonitor` dispatches `batch.completed` on terminal status transitions (distinguishing COMPLETED/FAILED/CANCELLED in both the in-app message and the success icon) and `blocker.created` on per-task BLOCKED transitions. `/execution` requests browser permission once on mount when permission is `'default'`. `/proof` dispatches `gate.run.failed` per failed gate when a proof run completes with `passed === false`. Known limitation: notifications only fire while `BatchExecutionMonitor` is mounted (cross-page background poller is out of scope; tracked for future work).
48+
- **Browser + in-app center (#559)**: `useNotifications` hook with workspace-scoped `localStorage` persistence and browser Notification dispatch (only when tab hidden + permission granted); `NotificationProvider` in root layout; `NotificationCenter` (bell icon + dropdown) mounts in sidebar footer. `/execution` requests browser permission once on mount when permission is `'default'`. `/proof` dispatches `gate.run.failed` per failed gate when a proof run completes with `passed === false`. **Background delivery (#652)**: a cross-page watcher (`useBatchNotificationWatcher`, mounted once in `NotificationProvider` in the root layout so it runs on every route) polls `GET /api/v2/batches` and is the single dispatcher of `batch.completed` (terminal transitions, distinguishing COMPLETED/FAILED/CANCELLED) and `blocker.created` (per-task BLOCKED transitions) — so these fire even when `BatchExecutionMonitor` is unmounted. The watcher baselines on its first poll (no spurious alerts for already-terminal/blocked batches), resets on workspace change, and guards against overlapping in-flight polls; `BatchExecutionMonitor` no longer dispatches them (avoids duplicates). Remaining limitation: `gate.run.failed` stays page-scoped to `/proof` (a proof run is a synchronous request/response the user actively watches, not a server-tracked background job).
4949
- **Outbound webhook (#560)**: Settings → Notifications tab takes a single URL + enabled toggle, persisted to `.codeframe/notifications_config.json` via `atomic_write_json`. `GET/PUT /api/v2/settings/notifications` and `POST /api/v2/settings/notifications/test` (test fires a sample payload and surfaces status code). `WebhookNotificationService.send_event` is the generic backend; dispatched fire-and-forget (5s timeout) from `core/conductor.py` on `BATCH_COMPLETED` only (not PARTIAL/FAILED/CANCELLED), `core/blockers.py:create()` after `BLOCKER_CREATED`, and `ui/routers/pr_v2.py:merge_pull_request` after successful merge. Failures are logged but never break the triggering operation.
5050

5151
**Phase 5.2 is complete** — Costs page now ships per-task and per-agent breakdowns (#558) on top of the spend summary (#557). Backend: `GET /api/v2/costs/tasks?days=N&limit=M` (top-N tasks with titles, agent, tokens, cost) and `GET /api/v2/costs/by-agent?days=N` (per-agent rollup + total input/output tokens), both via `TokenRepository.get_top_tasks_by_cost` and `get_costs_by_agent`. Task board cards show an inline `MoneyBag02Icon` cost badge with token-breakdown tooltip when cost data exists. Fixed a v2 data-loss bug where `react_agent` int-cast UUID task IDs and stored NULL in `token_usage`.
Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,239 @@
1+
import { renderHook, act, waitFor } from '@testing-library/react';
2+
import { useBatchNotificationWatcher } from '@/hooks/useBatchNotificationWatcher';
3+
import { batchesApi, tasksApi } from '@/lib/api';
4+
import { getSelectedWorkspacePath } from '@/lib/workspace-storage';
5+
import type { BatchListResponse, BatchResponse, Task } from '@/types';
6+
7+
jest.mock('@/lib/api');
8+
jest.mock('@/lib/workspace-storage');
9+
10+
const mockList = batchesApi.list as jest.MockedFunction<typeof batchesApi.list>;
11+
const mockGetTask = tasksApi.getOne as jest.MockedFunction<typeof tasksApi.getOne>;
12+
const mockGetWorkspacePath = getSelectedWorkspacePath as jest.MockedFunction<
13+
typeof getSelectedWorkspacePath
14+
>;
15+
16+
function batch(overrides: Partial<BatchResponse> = {}): BatchResponse {
17+
return {
18+
id: 'batch-1234abcd',
19+
workspace_id: 'ws-1',
20+
task_ids: ['t1'],
21+
status: 'RUNNING',
22+
strategy: 'serial',
23+
max_parallel: 1,
24+
on_failure: 'continue',
25+
started_at: null,
26+
completed_at: null,
27+
results: { t1: 'IN_PROGRESS' },
28+
...overrides,
29+
};
30+
}
31+
32+
function listResponse(batches: BatchResponse[]): BatchListResponse {
33+
return { batches, total: batches.length, by_status: {} };
34+
}
35+
36+
// Queue a sequence of list() responses, one per poll tick.
37+
function queueResponses(...responses: BatchListResponse[]) {
38+
mockList.mockReset();
39+
responses.forEach((r) => mockList.mockResolvedValueOnce(r));
40+
// Any further polls repeat the last response.
41+
if (responses.length > 0) {
42+
mockList.mockResolvedValue(responses[responses.length - 1]);
43+
}
44+
}
45+
46+
const INTERVAL = 1000;
47+
48+
beforeEach(() => {
49+
jest.useFakeTimers();
50+
jest.clearAllMocks();
51+
mockGetWorkspacePath.mockReturnValue('/ws');
52+
mockGetTask.mockResolvedValue({ id: 't1', title: 'Build login form' } as Task);
53+
});
54+
55+
afterEach(() => {
56+
jest.runOnlyPendingTimers();
57+
jest.useRealTimers();
58+
});
59+
60+
/** Run the immediate mount poll + flush its async work. */
61+
async function flushPoll() {
62+
await act(async () => {
63+
await Promise.resolve();
64+
await Promise.resolve();
65+
});
66+
}
67+
68+
/** Advance one polling interval and flush async work. */
69+
async function tick() {
70+
await act(async () => {
71+
jest.advanceTimersByTime(INTERVAL);
72+
await Promise.resolve();
73+
await Promise.resolve();
74+
});
75+
}
76+
77+
describe('useBatchNotificationWatcher', () => {
78+
it('does not notify for batches already terminal on the first poll (baseline)', async () => {
79+
const addNotification = jest.fn();
80+
queueResponses(listResponse([batch({ status: 'COMPLETED', results: { t1: 'COMPLETED' } })]));
81+
82+
renderHook(() => useBatchNotificationWatcher(addNotification, { intervalMs: INTERVAL }));
83+
await flushPoll();
84+
85+
expect(addNotification).not.toHaveBeenCalled();
86+
});
87+
88+
it('fires batch.completed when a running batch transitions to a terminal state', async () => {
89+
const addNotification = jest.fn();
90+
queueResponses(
91+
listResponse([batch({ status: 'RUNNING', results: { t1: 'IN_PROGRESS' } })]),
92+
listResponse([batch({ status: 'COMPLETED', results: { t1: 'COMPLETED' } })])
93+
);
94+
95+
renderHook(() => useBatchNotificationWatcher(addNotification, { intervalMs: INTERVAL }));
96+
await flushPoll(); // baseline = RUNNING
97+
expect(addNotification).not.toHaveBeenCalled();
98+
99+
await tick(); // now COMPLETED
100+
101+
expect(addNotification).toHaveBeenCalledTimes(1);
102+
expect(addNotification).toHaveBeenCalledWith(
103+
expect.objectContaining({
104+
type: 'batch.completed',
105+
batchStatus: 'COMPLETED',
106+
batchId: 'batch-1234abcd',
107+
})
108+
);
109+
});
110+
111+
it('fires batch.completed only once across repeated polls', async () => {
112+
const addNotification = jest.fn();
113+
queueResponses(
114+
listResponse([batch({ status: 'RUNNING', results: { t1: 'IN_PROGRESS' } })]),
115+
listResponse([batch({ status: 'FAILED', results: { t1: 'FAILED' } })])
116+
);
117+
118+
renderHook(() => useBatchNotificationWatcher(addNotification, { intervalMs: INTERVAL }));
119+
await flushPoll();
120+
await tick(); // FAILED
121+
await tick(); // still FAILED — must not re-fire
122+
123+
expect(addNotification).toHaveBeenCalledTimes(1);
124+
expect(addNotification).toHaveBeenCalledWith(
125+
expect.objectContaining({ type: 'batch.completed', batchStatus: 'FAILED' })
126+
);
127+
});
128+
129+
it('fires blocker.created with the task title when a task transitions to BLOCKED', async () => {
130+
const addNotification = jest.fn();
131+
queueResponses(
132+
listResponse([batch({ status: 'RUNNING', results: { t1: 'IN_PROGRESS' } })]),
133+
listResponse([batch({ status: 'RUNNING', results: { t1: 'BLOCKED' } })])
134+
);
135+
136+
renderHook(() => useBatchNotificationWatcher(addNotification, { intervalMs: INTERVAL }));
137+
await flushPoll();
138+
await tick();
139+
140+
await waitFor(() =>
141+
expect(addNotification).toHaveBeenCalledWith(
142+
expect.objectContaining({
143+
type: 'blocker.created',
144+
taskId: 't1',
145+
message: expect.stringContaining('Build login form'),
146+
})
147+
)
148+
);
149+
});
150+
151+
it('does nothing when no workspace is selected', async () => {
152+
const addNotification = jest.fn();
153+
mockGetWorkspacePath.mockReturnValue(null);
154+
queueResponses(listResponse([batch()]));
155+
156+
renderHook(() => useBatchNotificationWatcher(addNotification, { intervalMs: INTERVAL }));
157+
await flushPoll();
158+
159+
expect(mockList).not.toHaveBeenCalled();
160+
expect(addNotification).not.toHaveBeenCalled();
161+
});
162+
163+
it('does not start an overlapping poll while one is still in flight', async () => {
164+
const addNotification = jest.fn();
165+
// First list() never resolves during the test window — simulates a slow poll.
166+
let resolveSlow: (v: BatchListResponse) => void = () => {};
167+
const slow = new Promise<BatchListResponse>((res) => {
168+
resolveSlow = res;
169+
});
170+
mockList.mockReset();
171+
mockList.mockReturnValueOnce(slow);
172+
mockList.mockResolvedValue(listResponse([batch()]));
173+
174+
renderHook(() => useBatchNotificationWatcher(addNotification, { intervalMs: INTERVAL }));
175+
await flushPoll(); // immediate poll starts, awaiting `slow`
176+
await tick(); // interval fires but must be skipped (in-flight)
177+
await tick();
178+
179+
// Only the one still-pending call was made; no overlap.
180+
expect(mockList).toHaveBeenCalledTimes(1);
181+
182+
// Let the slow poll finish; subsequent ticks resume normally.
183+
await act(async () => {
184+
resolveSlow(listResponse([batch()]));
185+
await Promise.resolve();
186+
await Promise.resolve();
187+
});
188+
await tick();
189+
expect(mockList.mock.calls.length).toBeGreaterThan(1);
190+
});
191+
192+
it('does not dispatch stale notifications when the workspace changes mid-poll', async () => {
193+
const addNotification = jest.fn();
194+
// A slow poll for workspace /ws-a that resolves with a terminal transition.
195+
let resolveSlow: (v: BatchListResponse) => void = () => {};
196+
const slow = new Promise<BatchListResponse>((res) => {
197+
resolveSlow = res;
198+
});
199+
mockGetWorkspacePath.mockReturnValue('/ws-a');
200+
mockList.mockReset();
201+
// First poll (baseline) sees RUNNING; second poll returns the slow promise.
202+
mockList.mockResolvedValueOnce(
203+
listResponse([batch({ status: 'RUNNING', results: { t1: 'IN_PROGRESS' } })])
204+
);
205+
mockList.mockReturnValueOnce(slow);
206+
mockList.mockResolvedValue(listResponse([batch()]));
207+
208+
renderHook(() => useBatchNotificationWatcher(addNotification, { intervalMs: INTERVAL }));
209+
await flushPoll(); // baseline RUNNING for /ws-a
210+
await tick(); // second poll starts, awaiting `slow`
211+
212+
// Workspace switches away before the slow poll resolves.
213+
mockGetWorkspacePath.mockReturnValue('/ws-b');
214+
await act(async () => {
215+
resolveSlow(listResponse([batch({ status: 'COMPLETED', results: { t1: 'COMPLETED' } })]));
216+
await Promise.resolve();
217+
await Promise.resolve();
218+
});
219+
220+
// The terminal transition belongs to /ws-a, which is no longer active.
221+
expect(addNotification).not.toHaveBeenCalled();
222+
});
223+
224+
it('stops polling after unmount', async () => {
225+
const addNotification = jest.fn();
226+
queueResponses(listResponse([batch()]));
227+
228+
const { unmount } = renderHook(() =>
229+
useBatchNotificationWatcher(addNotification, { intervalMs: INTERVAL })
230+
);
231+
await flushPoll();
232+
const callsBefore = mockList.mock.calls.length;
233+
234+
unmount();
235+
await tick();
236+
237+
expect(mockList.mock.calls.length).toBe(callsBefore);
238+
});
239+
});

web-ui/src/components/execution/BatchExecutionMonitor.tsx

Lines changed: 4 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import {
2424
import { batchesApi, tasksApi } from '@/lib/api';
2525
import { EventStream } from './EventStream';
2626
import { useExecutionMonitor } from '@/hooks/useExecutionMonitor';
27-
import { useNotificationContext } from '@/contexts/NotificationContext';
2827
import type { BatchResponse, Task } from '@/types';
2928

3029
// ── Status icon helper ────────────────────────────────────────────────
@@ -51,7 +50,6 @@ interface BatchExecutionMonitorProps {
5150

5251
export function BatchExecutionMonitor({ batchId, workspacePath }: BatchExecutionMonitorProps) {
5352
const router = useRouter();
54-
const { addNotification } = useNotificationContext();
5553
const [batch, setBatch] = useState<BatchResponse | null>(null);
5654
const [tasks, setTasks] = useState<Record<string, Task>>({});
5755
const [expandedTaskId, setExpandedTaskId] = useState<string | null>(null);
@@ -61,10 +59,6 @@ export function BatchExecutionMonitor({ batchId, workspacePath }: BatchExecution
6159
// Track which task IDs have already been fetched to avoid refetching
6260
const fetchedTaskIdsRef = useRef<Set<string>>(new Set());
6361

64-
// Track previous batch + per-task statuses for transition-based notifications
65-
const prevBatchStatusRef = useRef<string | null>(null);
66-
const prevTaskStatusesRef = useRef<Record<string, string>>({});
67-
6862
// ── Fetch batch details + task names ────────────────────────────────
6963
const fetchBatch = useCallback(async () => {
7064
try {
@@ -104,55 +98,10 @@ export function BatchExecutionMonitor({ batchId, workspacePath }: BatchExecution
10498
};
10599
}, [batch?.status, fetchBatch]);
106100

107-
// Fire notifications on batch terminal transition + per-task BLOCKED transitions
108-
useEffect(() => {
109-
if (!batch) return;
110-
111-
const TERMINAL = ['COMPLETED', 'FAILED', 'CANCELLED'];
112-
const prevBatchStatus = prevBatchStatusRef.current;
113-
if (
114-
prevBatchStatus !== null &&
115-
!TERMINAL.includes(prevBatchStatus) &&
116-
TERMINAL.includes(batch.status)
117-
) {
118-
const completedCount = batch.task_ids.filter(
119-
(id) => batch.results[id] === 'COMPLETED' || batch.results[id] === 'DONE'
120-
).length;
121-
const total = batch.task_ids.length;
122-
const shortId = batchId.slice(0, 8);
123-
const outcomeMessage =
124-
batch.status === 'COMPLETED'
125-
? `Batch ${shortId} finished — ${completedCount}/${total} tasks done`
126-
: batch.status === 'FAILED'
127-
? `Batch ${shortId} failed — ${completedCount}/${total} tasks completed before failure`
128-
: `Batch ${shortId} cancelled — ${completedCount}/${total} tasks completed`;
129-
addNotification({
130-
type: 'batch.completed',
131-
batchStatus: batch.status as 'COMPLETED' | 'FAILED' | 'CANCELLED',
132-
message: outcomeMessage,
133-
batchId,
134-
});
135-
}
136-
prevBatchStatusRef.current = batch.status;
137-
138-
// Per-task: notify on transition to BLOCKED
139-
const prevTaskStatuses = prevTaskStatusesRef.current;
140-
for (const taskId of batch.task_ids) {
141-
const currentStatus = batch.results[taskId];
142-
const prevStatus = prevTaskStatuses[taskId];
143-
if (currentStatus === 'BLOCKED' && prevStatus && prevStatus !== 'BLOCKED') {
144-
const title = tasks[taskId]?.title;
145-
addNotification({
146-
type: 'blocker.created',
147-
message: title
148-
? `Agent is blocked on "${title}" — your input needed`
149-
: 'Agent is blocked — your input needed',
150-
taskId,
151-
});
152-
}
153-
prevTaskStatuses[taskId] = currentStatus ?? 'READY';
154-
}
155-
}, [batch, batchId, tasks, addNotification]);
101+
// Note: batch.completed / blocker.created notifications are dispatched by the
102+
// cross-page background watcher in NotificationProvider (issue #652), so they
103+
// fire even when this monitor is unmounted. This component only renders the
104+
// live view; it no longer dispatches notifications to avoid duplicates.
156105

157106
// Auto-expand the first IN_PROGRESS task
158107
useEffect(() => {

web-ui/src/contexts/NotificationContext.tsx

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,28 @@
22

33
import { createContext, useContext, type ReactNode } from 'react';
44
import { useNotifications, type UseNotificationsReturn } from '@/hooks/useNotifications';
5+
import { useBatchNotificationWatcher } from '@/hooks/useBatchNotificationWatcher';
56

67
const NotificationContext = createContext<UseNotificationsReturn | null>(null);
78

9+
/**
10+
* Cross-page background watcher. Mounted once here (inside the provider, so it
11+
* runs on every route) it is the single dispatcher of batch.completed and
12+
* blocker.created — making those notifications fire even when the execution
13+
* page is unmounted. See issue #652.
14+
*/
15+
function BackgroundBatchWatcher({ addNotification }: { addNotification: UseNotificationsReturn['addNotification'] }) {
16+
useBatchNotificationWatcher(addNotification);
17+
return null;
18+
}
19+
820
export function NotificationProvider({ children }: { children: ReactNode }) {
921
const value = useNotifications();
1022
return (
11-
<NotificationContext.Provider value={value}>{children}</NotificationContext.Provider>
23+
<NotificationContext.Provider value={value}>
24+
<BackgroundBatchWatcher addNotification={value.addNotification} />
25+
{children}
26+
</NotificationContext.Provider>
1227
);
1328
}
1429

0 commit comments

Comments
 (0)