📋 feat: View and Stop Background Tasks From the Chat Header - #16348
danny-avila wants to merge 3 commits into
Conversation
867e351 to
2ff460d
Compare
2ff460d to
9e9ae79
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e9ae79ce5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const args = useMemo(() => { | ||
| const ids = new Set((data?.tasks ?? []).map((task) => task.toolCallId)); | ||
| const messages = queryClient.getQueryData<TMessage[]>([QueryKeys.messages, conversationId]); | ||
| return findToolCallArgs(messages, ids); |
There was a problem hiding this comment.
Provider tool-call IDs can repeat across turns and agents, but this lookup is keyed only by toolCallId and scans newest-first. When two retained tasks share an ID such as call_0, both rows receive the newest call's intent and command while their stop controls still target distinct task IDs, so a user can inspect one command and cancel another. Include the returned messageId or another dispatch identity in the lookup key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d1c7653: task details now key tool-call arguments by dispatch message ID plus tool-call ID, with a fallback only for legacy rows without a message ID. Added duplicate-ID regression coverage.
| if (index?.tasks.some((task) => task.status === 'running') === true) { | ||
| return RUNNING_REFRESH_MS; | ||
| } | ||
| return isSubmitting ? SUBMITTING_REFRESH_MS : false; |
There was a problem hiding this comment.
If the cached response is empty, isSubmitting only enables a five-second timer; it does not immediately refetch. When a background task is dispatched less than five seconds before the foreground run ends, the next render changes this interval back to false before any request sees the task, and nothing else invalidates this query, leaving the header button hidden until a window-focus or remount. Add a bounded post-submit discovery poll or explicitly refetch on the submission transition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d1c7653: the query refetches on submission completion and keeps a bounded post-submit discovery poll. Added polling transition coverage.
| setStoppingThreads( | ||
| (current) => new Set([...current, ...subagents.map((target) => target.threadId)]), | ||
| ); | ||
| const submittedAt = new Date().toISOString(); | ||
| await Promise.allSettled([ |
There was a problem hiding this comment.
When a subagent cancellation rejects at the HTTP layer or resolves with a rejected receipt, its thread is first added to stoppingThreads, but the Promise.allSettled outcomes are discarded and that ID is never removed. After refresh the still-running child continues to render as stopping, which makes canStop false and removes every retry control for the lifetime of the component while the work continues. Reconcile each outcome, clear failed or rejected IDs, and surface the failure.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d1c7653: rejected cancellation outcomes clear optimistic stopping IDs and surface a retryable failure state. Added focused client coverage.
| ...(settled | ||
| ? { settledAt: new Date(Math.max(task.updatedAt, task.createdAt)).toISOString() } | ||
| : {}), |
There was a problem hiding this comment.
updatedAt is not the task's terminal time: settled tasks update it later when completion persistence finishes, results are claimed or released, artifacts are restored, or wakeups are retired. Consequently settledAt can represent a much later poll or delivery event, causing the card's elapsed duration to substantially overstate how long the tool ran. Record a terminal timestamp in complete, fail, and cancel instead of projecting mutable updatedAt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d1c7653: terminal transitions now record settledAt, which the summary projects instead of mutable updatedAt. Added route/registry coverage.
| const requested = (req.body as { taskIds?: unknown } | undefined)?.taskIds; | ||
| if ( | ||
| requested != null && | ||
| (!Array.isArray(requested) || |
There was a problem hiding this comment.
A malformed payload such as {"taskIds": null}, a scalar, or an array causes this extraction to produce null or undefined, skips validation, and then enters the cancel-all branch. Thus a client serialization error can cancel every running task even though the API contract reserves that behavior for an omitted taskIds property. Validate that the body is an object and distinguish an absent property from invalid null or non-object input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d1c7653: cancel-all now requires an object body with taskIds omitted; null, arrays, and malformed taskIds receive 400. Added route tests.
| {!isNewChat && parentConversationId == null && ( | ||
| <BackgroundTasksButton conversationId={routeConversationId} isSubmitting={isSubmitting} /> |
There was a problem hiding this comment.
This branch mounts the real background-task component in client/src/components/Chat/__tests__/Header.stacking.spec.tsx, where the route is convo-1, the direct render(<Header />) has no query provider, and the ~/data-provider mock exposes only useGetStartupConfig. The component therefore calls an undefined useBackgroundTasksQuery before the stacking assertion, breaking that existing test; mock the new control or supply its hooks and QueryClientProvider.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d1c7653: the Header stacking test now supplies the new background-task hook dependencies. The focused Header spec passes locally.
| const anyStoppable = running.some(view.canStop); | ||
| const stopAllLabel = anyStoppable | ||
| ? localize('com_ui_background_tasks_stop_all') | ||
| : localize('com_ui_background_tasks_cancel_disabled'); |
There was a problem hiding this comment.
When ordinary-tool cancellation is disabled but at least one subagent is stoppable, anyStoppable is true and this button is labeled “Stop all running tasks.” However, stopAll filters through canStop, so it cancels only the subagents and silently leaves the running ordinary tools untouched. Use a label that reflects the partial scope or otherwise tell the user which running tasks cannot be stopped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d1c7653: mixed stoppability uses a localized 'stop available tasks' label rather than promising to stop all running work.
| <button | ||
| type="button" | ||
| aria-label={stopAllLabel} | ||
| disabled={!anyStoppable || view.isStopping} | ||
| onClick={() => void view.stopAll()} | ||
| data-testid="background-tasks-stop-all" | ||
| className={cn(iconButtonClass, 'border border-border-medium')} |
There was a problem hiding this comment.
When no running task is stoppable, this button is disabled, while iconButtonClass also applies disabled:pointer-events-none. Because TooltipAnchor installs its hover and focus behavior on this same rendered button, pointer events cannot reach it and a disabled native button cannot receive keyboard focus, so the only visible explanation for why stopping is unavailable can never appear. Put the disabled button inside a focusable tooltip anchor or otherwise expose the reason.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d1c7653: the disabled stop button now sits inside a focusable tooltip anchor so the explanation remains reachable.
| className={cn( | ||
| 'z-[200] flex max-h-[min(36rem,calc(100vh-5rem))] max-w-[calc(100vw-2rem)] flex-col rounded-xl border border-border-medium bg-surface-secondary text-text-primary shadow-lg focus:outline-none', | ||
| wide ? 'w-[36rem]' : 'w-80', |
There was a problem hiding this comment.
The shared TooltipAnchor portals its popup to the document with .tooltip { z-index: 150 }, but this new popover is assigned z-[200]. Consequently the stop-all and per-task stop tooltips rendered from inside this surface paint underneath the opaque popover, making them invisible where they overlap it. Supply the appropriate portal or depth-aware z-index rather than placing the containing popover above the shared tooltip layer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d1c7653: stop tooltips now portal into the task popover instead of painting beneath it.
| {!isNewChat && parentConversationId == null && ( | ||
| <BackgroundTasksButton conversationId={routeConversationId} isSubmitting={isSubmitting} /> |
There was a problem hiding this comment.
This mounts BackgroundTasksButton for every saved parent conversation, while its query is enabled solely from the conversation ID. The only registry producers and cancellation configuration live in the agents execution path, so ordinary non-agent chats can never return a task yet now issue this config-backed GET on mount and every window focus. Gate the component or query on the effective agents endpoint to avoid adding an unnecessary request to all other conversation views.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d1c7653: the Header mounts background-task controls only for the effective Agents endpoint.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fa7ee2f1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| now: number; | ||
| }): BackgroundTasksView { | ||
| const queryClient = useQueryClient(); | ||
| const { data } = useBackgroundTasksQuery(conversationId, undefined, isSubmitting); |
There was a problem hiding this comment.
When the initial background-task request fails, this discards the query's error state and builds an empty row list, so BackgroundTasksButton returns null. Once React Query's retries finish, a user with running ordinary tools has neither an error message nor a manual retry/cancel entry point until a focus change or remount. Surface the failure and provide a retry action rather than treating it as an empty result.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
| const args = useMemo(() => { | ||
| const messages = queryClient.getQueryData<TMessage[]>([QueryKeys.messages, conversationId]); | ||
| return findToolCallArgs(messages, data?.tasks ?? []); | ||
| }, [data?.tasks, conversationId, queryClient]); |
There was a problem hiding this comment.
On a restored session, the task index can resolve before the messages query finishes loading. This memo reads the cache only when data.tasks changes; unchanged running-task responses are structurally shared by React Query, and this hook is not subscribed to the messages query, so the later message load never fills in the task intent or command while the task is running. Subscribe to the relevant message data or otherwise recompute when that cache entry becomes available.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
| setStoppingThreads( | ||
| (current) => new Set([...current, ...subagents.map((target) => target.threadId)]), | ||
| ); |
There was a problem hiding this comment.
When a child thread advances from cancelled task A to a queued task B, this state still records only the thread ID. The refreshed summary then exposes B as the running latestTaskId, but buildTaskRows marks every running task on that thread as stopping, and the cleanup effect retains the entry while the thread remains running; B therefore has no stop control for its entire run. Key the optimistic state by both thread and task ID, or clear it when latestTaskId changes.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
| useEffect(() => { | ||
| if (!open || activeCount === 0) return; | ||
| setNow(Date.now()); | ||
| const timer = setInterval(() => setNow(Date.now()), TICK_MS); | ||
| return () => clearInterval(timer); |
There was a problem hiding this comment.
When activeCount becomes zero, this effect stops the timer and never updates now again, even when the popover is opened. Because buildTaskRows uses that frozen value to enforce RECENT_SUBAGENT_WINDOW_MS, a subagent that settles while the tab is mounted remains classified as recent indefinitely and keeps the header control and finished row visible past the intended one-hour retention window. Schedule an expiry update for settled rows or refresh now whenever the popover opens.
Useful? React with 👍 / 👎.
| subagentControlHandler, | ||
| ); | ||
| router.get('/:parentConversationId/subagents', parentSubagentIndexHandler); | ||
| router.get('/:conversationId/background-tasks', configMiddleware, backgroundTaskIndexHandler); |
There was a problem hiding this comment.
In deployments with statefulCodeSessions configured, this middleware turns every background-task status poll into database work: configMiddleware calls getAppConfig, cached merged configs are still runtime-augmented, and mergeAccessibleCodeEnvironments rechecks ACLs and live environments with Mongo queries on every use. Since the client hits this route every two seconds while a task runs, concurrent tasks can generate sustained database traffic merely to read an in-memory registry and one cancellation flag. Load the narrow background-task policy without runtime code-environment augmentation, or pass already-resolved request configuration into this path.
AGENTS.md reference: AGENTS.md:L51-L53
Useful? React with 👍 / 👎.
This branch has not been deployed
2 participants
Summary
A conversation's background work had no single place to look at it. Ordinary background tools (bash, code) showed only on the tool cards that started them. Detached subagents showed only in their own threads. No HTTP route could list ordinary background tools or stop them. The only stop was the model's
check_background_tasktool.This adds a header button that appears when the conversation has background tasks, with a dot while any are running. It opens a popover with:
Tool rows come from the process-local background registry. Subagent rows come from the existing parent subagent index: running ones, plus those settled within the registry's one-hour retention. Stop-all cancels ordinary tools through the new route and subagents through the existing control route (
action: 'cancel').Mechanism
packages/api/src/agents/tasks.tsholds the handlers. They receive the registry from the route, so the spec runs against a realBackgroundTaskRegistryClass.userId::conversationId, so another user's tasks cannot be reached.taskIdstargets every running task. Each task reports the registry outcome:requested,already_requested,settled,not_foundorunavailable.endpoints.agents.backgroundTasks.ordinaryToolCancellation, which is off by default. When it is off, cancel returns 403 and the popover disables stop for tool rows, with a tooltip saying why. Subagent stop is not gated by it.toolCallIdwhen the task list changes, rather than through a subscription, so streaming does not re-render the header.Testing
packages/api:src/agents/tasks.spec.ts, 5 tests. Covers listing without results, per-user isolation, cancel-all vs cancel-by-id outcomes, the opt-in gate, and malformed input.client:BackgroundTasks/__tests__, 6 tests. Covers:clientParentSubagentsProvidertests still pass.clienttsc --noEmit: clean apart from the pre-existing sandpack errors in this environment.packages/apitsc: no errors in touched files.