fix(web-ui): deliver batch notifications when execution monitor is unmounted (#652)

[frankbria]

Summary

Closes #652.

Async batch.completed / blocker.created notifications previously only fired while BatchExecutionMonitor was mounted — i.e. only on /execution?batch=…. Navigate away and a background batch that finishes produces no notification (in-app or browser), defeating the whole point of browser alerts. This was a known limitation called out in CLAUDE.md.

This implements the issue's preferred fix: a cross-page background poller.

What changed

• New hook useBatchNotificationWatcher (web-ui/src/hooks/useBatchNotificationWatcher.ts): polls the existing GET /api/v2/batches endpoint on a steady interval (default 10s). It:
  • Establishes a baseline on the first poll so already-terminal / already-blocked batches don't emit a spurious notification.
  • Fires batch.completed on a batch's transition into a terminal state (COMPLETED/FAILED/CANCELLED) and blocker.created on a per-task transition into BLOCKED (lazily fetching the task title for a friendly message).
  • Resets its baseline when the active workspace changes.
  • Uses an in-flight guard so a slow poll can't overlap the next tick and corrupt the transition baseline (double-fire / re-arm).
• Mounted once inside NotificationProvider (contexts/NotificationContext.tsx), which lives in the root layout and does not remount on navigation — so the watcher runs on every route and its baseline persists across page changes.
• Single source of truth: removed the batch.completed / blocker.created dispatch from BatchExecutionMonitor to avoid double notifications on /execution. The monitor keeps its live visual polling/display.
• API: added batchesApi.list() + BatchListResponse type. No backend change — the list endpoint already returns per-task results.

Testing

• New useBatchNotificationWatcher.test.ts (7 tests): first-poll baseline suppression, terminal transition, fire-once, BLOCKED + title fetch, no-workspace no-op, in-flight overlap guard, unmount cleanup.
• Full web-ui suite: 1012 tests pass; npm run build succeeds; eslint clean on changed files.

Known limitations

• gate.run.failed on /proof remains page-scoped. Unlike batches, a proof run is a synchronous request/response action the user actively watches on that page — there's no server-tracked background job to poll — so the navigate-away problem doesn't apply there. Left as-is.
• PARTIAL batch status does not emit a batch.completed notification, matching the pre-existing monitor behavior and the webhook dispatch scope (which intentionally excludes PARTIAL).

Summary by CodeRabbit

• New Features
  • Batch completion and blocker notifications are now delivered by a background watcher, so they keep firing across page navigation and after the batch view unmounts.
  • Blocker notifications include the related task title for clearer context.
• Bug Fixes
  • Improved notification reliability by preventing duplicate/overlapping polling and avoiding stale notifications when the selected workspace changes.
• Tests
  • Expanded Jest coverage for polling, state transitions, single-fire behavior, and safe cleanup.
• Documentation
  • Updated async notification docs to reflect the cross-page delivery approach.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c8f63c7e-3567-4cc1-8a5e-2b5019e12248

📥 Commits

Reviewing files that changed from the base of the PR and between 81f2856 and b183c78.

📒 Files selected for processing (2)

• web-ui/src/__tests__/hooks/useBatchNotificationWatcher.test.ts
• web-ui/src/hooks/useBatchNotificationWatcher.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• web-ui/src/tests/hooks/useBatchNotificationWatcher.test.ts
• web-ui/src/hooks/useBatchNotificationWatcher.ts

---

Walkthrough

Batch completion and blocker notifications are moved from BatchExecutionMonitor (which only ran on the execution page) into a new useBatchNotificationWatcher hook mounted permanently inside NotificationProvider. A new batchesApi.list method and BatchListResponse type back the polling. A full test suite covers the hook's transition logic, overlap guard, stale-result handling, and cleanup.

Changes

Cross-page Background Batch Watcher

Layer / File(s)	Summary
BatchListResponse type and batchesApi.list method web-ui/src/types/index.ts, web-ui/src/lib/api.ts	Adds the BatchListResponse interface (batches, total, by_status) and the batchesApi.list(workspacePath, options?) method calling GET /api/v2/batches with optional status and limit filters.
useBatchNotificationWatcher polling hook web-ui/src/hooks/useBatchNotificationWatcher.ts	New use client hook that polls workspace batches on a configurable interval (default 10s), guards against overlapping polls, resets transition baselines on workspace change, fires batch.completed on batch terminal-status transitions and blocker.created on per-task BLOCKED transitions; includes buildBatchMessage and notifyBlocked helpers.
NotificationProvider wiring and BatchExecutionMonitor cleanup web-ui/src/contexts/NotificationContext.tsx, web-ui/src/components/execution/BatchExecutionMonitor.tsx	NotificationProvider renders an internal BackgroundBatchWatcher component that calls useBatchNotificationWatcher(addNotification); BatchExecutionMonitor removes useNotificationContext, previous-status refs, and the notification-triggering useEffect.
useBatchNotificationWatcher test suite web-ui/src/__tests__/hooks/useBatchNotificationWatcher.test.ts	Eight tests using fake timers and polling helpers cover: no notification on initial terminal poll, batch.completed on transition, no re-fire on repeated terminal state, blocker.created with task title, no-op without a workspace, overlap prevention for in-flight polls, stale-notification suppression when workspace changes mid-poll, and polling stop after unmount.
Documentation update CLAUDE.md	Phase 5.3 section updated to reflect cross-page background notification dispatch via useBatchNotificationWatcher mounted in NotificationProvider, clarifying that BatchExecutionMonitor no longer dispatches batch.completed/blocker.created.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• frankbria/codeframe#593: Originally introduced batch.completed/blocker.created notification dispatch inside BatchExecutionMonitor, which this PR extracts into the new background watcher mounted at the provider level.

Poem

🐇 Hop, hop, across every page I go,
No longer tied to one route's show.
My watcher polls from the provider's nest,
Alerting batches — completed, blocked, distressed.
Background notifs now never sleep,
useBatchNotificationWatcher — mine to keep! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Title clearly identifies the main change: delivering batch notifications when the execution monitor is unmounted, which is the core objective of this PR.
Linked Issues check	✅ Passed	The PR fully satisfies issue #652 by implementing the preferred solution: a cross-page background poller mounted in NotificationProvider that continuously monitors batch state changes regardless of route.
Out of Scope Changes check	✅ Passed	All changes are directly related to the linked issue objective. The PR implements background notification delivery by adding the useBatchNotificationWatcher hook, integrating it into NotificationProvider, removing duplicate logic from BatchExecutionMonitor, and adding supporting API/type definitions.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/652-background-batch-notifications

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

web-ui/src/__tests__/hooks/useBatchNotificationWatcher.test.ts (1)

77-207: ⚡ Quick win

Add a regression test for workspace changes mid-poll.

Current coverage is strong, but it doesn’t assert behavior when getSelectedWorkspacePath() changes between poll start and response resolution. Add one case that flips workspace before resolving batchesApi.list and verifies no stale notification is dispatched.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-ui/src/__tests__/hooks/useBatchNotificationWatcher.test.ts` around lines
77 - 207, Add a new test case within the useBatchNotificationWatcher describe
block to verify behavior when the workspace changes between poll start and
response resolution. Create a test that mocks a slow list response (similar to
the "does not start an overlapping poll" test), starts the renderHook with
useBatchNotificationWatcher, calls flushPoll to initiate the pending list call,
then changes the workspace by calling mockGetWorkspacePath.mockReturnValue(null)
before resolving the slow promise, and finally asserts that addNotification was
never called with any notification (ensuring no stale data is dispatched when
the workspace becomes invalid mid-poll).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@web-ui/src/hooks/useBatchNotificationWatcher.ts`:
- Around line 73-113: The poll callback captures the workspace path at the start
but doesn't verify that the workspace hasn't changed after the async
batchesApi.list call completes. If the selected workspace changes while the
request is in flight, lines 86-110 (including the addRef.current notification
calls and the notifyBlocked call) will emit notifications from the stale
workspace context. Add a workspace validation check immediately after the
try-catch block (similar to the existing cancelled check on line 81) to ensure
the current workspace still matches the workspacePath captured at poll start,
and return early if it has changed.

---

Nitpick comments:
In `@web-ui/src/__tests__/hooks/useBatchNotificationWatcher.test.ts`:
- Around line 77-207: Add a new test case within the useBatchNotificationWatcher
describe block to verify behavior when the workspace changes between poll start
and response resolution. Create a test that mocks a slow list response (similar
to the "does not start an overlapping poll" test), starts the renderHook with
useBatchNotificationWatcher, calls flushPoll to initiate the pending list call,
then changes the workspace by calling mockGetWorkspacePath.mockReturnValue(null)
before resolving the slow promise, and finally asserts that addNotification was
never called with any notification (ensuring no stale data is dispatched when
the workspace becomes invalid mid-poll).

🪄 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: 91fa50d6-2617-4b53-bd5f-cfe1cc30b0e6

📥 Commits

Reviewing files that changed from the base of the PR and between 305726b and 96744f2.

📒 Files selected for processing (6)

• web-ui/src/__tests__/hooks/useBatchNotificationWatcher.test.ts
• web-ui/src/components/execution/BatchExecutionMonitor.tsx
• web-ui/src/contexts/NotificationContext.tsx
• web-ui/src/hooks/useBatchNotificationWatcher.ts
• web-ui/src/lib/api.ts
• web-ui/src/types/index.ts

[claude]

Code Review - PR 691: Cross-page batch notifications

First review - no prior comments from me on this PR.

Overview

Solid architecture fix for issue 652. Moving notification dispatch from BatchExecutionMonitor (page-scoped) to useBatchNotificationWatcher (root-layout-scoped) is the right call. The navigate-away blind spot is eliminated cleanly with no backend changes.

What is working well

• Single source of truth: removing the duplicate dispatch from BatchExecutionMonitor is correct. The comment left in the monitor explaining why will prevent future readers from re-adding it.
• Baseline suppression on first poll: the prevStatus === undefined guard is the right pattern - do not notify for already-terminal/blocked state that pre-dates the session.
• In-flight guard (pollingRef): prevents double-fire/re-arming when a poll is slow. The logic is correct.
• addRef pattern: capturing addNotification in a ref keeps the effect dependency list stable - correct and idiomatic.
• BackgroundBatchWatcher renderless component: neat pattern for mounting a hook inside a context provider without mixing lifecycle concerns into NotificationProvider itself.
• Test coverage: 7 focused tests with proper fake-timer hygiene cover the important paths - baseline, terminal transition, fire-once, BLOCKED+title, no-workspace, in-flight guard, and unmount.

Issues to address

1. notifyBlocked is not guarded by cancelled

The call void notifyBlocked(workspacePath, taskId, addRef.current) launches async work not protected by the cancelled flag. If the hook unmounts while a title-fetch is in flight, add still fires. In practice this is harmless because NotificationProvider lives in the root layout and rarely unmounts, but it is a silent invariant violation.

Fix: pass () => cancelled and addRef into notifyBlocked instead of the snapshot value, then add if (isCancelled()) return; after the title fetch completes.

2. pollingRef persists across effect re-runs (edge case)

pollingRef is defined in the hook body (not inside the effect), so it is shared across effect re-runs triggered by intervalMs changes. If a poll is in-flight when the interval changes, the new effect first poll call sees pollingRef.current === true and skips one tick. Not a correctness bug, but setting pollingRef.current = false in the effect cleanup would make re-triggering cleaner.

3. batchesApi.list silently drops limit: 0

The spread ...(options?.limit ? { limit: options.limit } : {}) silently ignores limit: 0 because 0 is falsy. For the actual call site ({ limit: 50 }) this is fine, but the helper makes a silent promise it does not keep. Prefer options?.limit != null as the guard.

Nits

• The hardcoded limit: 50 is fine for now. A brief comment noting that batches beyond position 50 may be missed if the list grows large would help.
• The JSDoc on BackgroundBatchWatcher duplicates the hook JSDoc almost verbatim. The hook JSDoc is the canonical place; the component comment could be trimmed to one line.

Test suggestions (nice-to-have, not blockers)

1. Workspace change resets baseline: verify a COMPLETED batch from workspace A does not fire a notification after switching to workspace B.
2. Cancelled + in-flight notifyBlocked: verify addNotification is not called after unmount when a title-fetch is still pending.

Verdict

The fix is architecturally correct and the implementation is clean. Items worth addressing before merge: the unguarded void notifyBlocked (issue 1) and the limit guard in api.ts (issue 3, low-effort). Issue 2 is minor. Tests are solid; the two suggested additions are nice-to-have.

Approve after issues 1 and 3 are addressed.

[claude]

Follow-up review — issues from my first review remain open

The second commit is CLAUDE.md-only (docs update for the known-limitation note). The hook and API code are unchanged, so all three items from my first review are still outstanding:

Still needs fixing before merge

1. notifyBlocked is not cancelled-aware (my issue 1, also flagged by CodeRabbit at lines 73–113)

   • void notifyBlocked(workspacePath, taskId, addRef.current) fires-and-forgets an async task-title fetch. If the component unmounts while that fetch is pending, add still calls into the stale context.
   • The cancelled flag is already present in the effect closure — it just needs to be threaded into notifyBlocked and checked after the tasksApi.getOne resolves.

2. batchesApi.list silently drops limit: 0 (my issue 3)

   • ...(options?.limit ? { limit: options.limit } : {}) treats 0 as falsy. The current call site (limit: 50) is fine, but the guard is misleading. Change to options?.limit != null.

Minor / can defer

3. pollingRef not reset on effect re-run (my issue 2) — very unlikely in practice but clean to add pollingRef.current = false in the effect cleanup.

Nice-to-have tests (still not blocking):

• Workspace-swap mid-poll — no stale notification (also requested by CodeRabbit).
• Unmount during in-flight notifyBlocked title fetch — confirm addNotification not called after unmount.

Nothing new to flag in the diff. The architecture is sound; these are the only items preventing a clean approval.

[frankbria]

Addressed CodeRabbit's review in b183c78:

• Major — stale poll after workspace switch: Verified valid. Added an isCurrentWorkspace() guard (!cancelled && getSelectedWorkspacePath() === workspacePath) checked after the batchesApi.list await and again inside notifyBlocked (before and after its title fetch), so a poll that resolves after the user switches workspaces no longer dispatches into the new workspace's notification store.
• Nitpick — regression test: Added does not dispatch stale notifications when the workspace changes mid-poll, which flips the workspace before resolving a slow in-flight poll and asserts no notification fires. Suite now 8/8.

[claude]

Follow-up review — after b183c78

Both items I flagged as "needs fixing before merge" in my first review are now resolved or effectively resolved:

Issue 1 — notifyBlocked not cancelled-aware: FIXED ✓

The isCurrentWorkspace() guard (closing over both cancelled and the captured workspacePath) is threaded correctly into notifyBlocked and checked twice — before the title fetch and after. The new test does not dispatch stale notifications when the workspace changes mid-poll covers this path and is well-constructed. This was the most important correctness issue and it's handled cleanly.

Issue 3 — batchesApi.list falsy guard on limit: still present, no longer blocking

...(options?.limit ? { limit: options.limit } : {}) still drops limit: 0 silently. The only call site is { limit: 50 }, so no actual behavior is affected. I still think options?.limit != null is the correct guard for a public-ish helper, but given the single call site I won't block on it.

Issue 2 — pollingRef not reset in effect cleanup: still present, still minor

As noted before, this is an edge-case cosmetic concern. pollingRef.current = false in the cleanup would make effect re-runs cleaner, but in practice intervalMs never changes and this has no observable consequence.

---

Summary: The architecture is sound, the main correctness bug is fixed, test coverage is solid (8 tests). The two remaining nits are genuinely minor and do not affect the shipped behavior. This is ready to merge.

[frankbria]

This is the same finding already addressed in b183c78 — the current code matches CodeRabbit's suggested diff exactly: isCurrentWorkspace() is checked after the batchesApi.list await (line 87) and both before and after the title fetch inside notifyBlocked (lines 159, 168). Resolved; no further change needed.