rewrite polling function #262947
rewrite polling function #262947
Conversation
meganrogge
force-pushed
the
merogge/rewrite
branch
from
9792a16 to
f2d9b55
Compare
meganrogge
force-pushed
the
merogge/rewrite
branch
from
f2d9b55 to
60c49b7
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the OutputMonitor polling functionality to use an event-driven approach instead of promise-based monitoring. The changes move from a startMonitoring() method that returns a result to one that fires events and exposes results via properties.
Key changes:
- Replace promise-based polling with event-driven monitoring that fires
onDidFinishCommandevents - Restructure the constructor to automatically start monitoring and accept monitoring parameters
- Extract common polling utilities into a new
bufferOutputPolling.tsfile
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
outputMonitor.test.ts |
Updated tests to use event-driven pattern and new constructor signature |
runInTerminalTool.ts |
Modified to use new OutputMonitor API with event listening instead of promise awaiting |
outputMonitor.ts |
Major refactor to event-driven polling with constructor-based initialization |
taskHelpers.ts |
Updated to use new OutputMonitor event-driven API |
bufferOutputPolling.ts |
New file containing extracted polling utility functions |
mcpSamplingService.ts |
Updated ExtensionIdentifier from 'core' to 'Github.copilot-chat' |
...s/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/monitoring/outputMonitor.ts
Show resolved
Hide resolved
...s/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/monitoring/outputMonitor.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/bufferOutputPolling.ts
Outdated
Show resolved
Hide resolved
…r/tools/monitoring/outputMonitor.ts Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
| // Ensure this method always runs truly async so listeners can attach | ||
| await Promise.resolve(); |
There was a problem hiding this comment.
await timeout(0) would be more clear here, I'm not sure how awaiting a resolved promise behaves exactly.
| // If we timed out, optionally ask to keep waiting | ||
| if (this._state === OutputMonitorState.Timeout) { |
There was a problem hiding this comment.
nit: switches are nicer when considering every state in a closed set of states like this imo
| private _pollingResult: IPollingResult & { pollDurationMs: number } | undefined; | ||
| get pollingResult(): IPollingResult & { pollDurationMs: number } | undefined { return this._pollingResult; } |
There was a problem hiding this comment.
Not sure if we can hide anything here but any details we can hide we should so that the OutputMonitor API to the outside is as simple as possible
| // Let custom poller override if provided | ||
| const custom = await pollFn?.(execution, token, this._taskService); | ||
| if (custom) { | ||
| return custom; | ||
| } | ||
|
|
||
| const modelOutputEvalResponse = await this._assessOutputForErrors(buffer, token); | ||
| return { output: buffer, state: this._state, modelOutputEvalResponse }; | ||
| } |
There was a problem hiding this comment.
Try splitting out the waiting for idle from the actioning and handling that in _startMonitoring. Can we leverage the waitForIdle helper here to simplify this?
...s/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/monitoring/outputMonitor.ts
Outdated
Show resolved
Hide resolved
…r/tools/monitoring/outputMonitor.ts Co-authored-by: Daniel Imms <[email protected]>
To do: