fix(webview): throttle state/message updates to prevent grey screens during long tasks #8609
Conversation
…s; throttle indexing updates to prevent grey screens during long tasks
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| if (!this.messageUpdateTimer) { | ||
| this.messageUpdateTimer = setTimeout(async () => { | ||
| try { | ||
| const batch = Array.from(this.messageUpdateBuffer.values()) | ||
| this.messageUpdateBuffer.clear() | ||
| this.messageUpdateTimer = undefined | ||
| for (const m of batch) { | ||
| await provider.postMessageToWebview({ type: "messageUpdated", clineMessage: m }) | ||
| } | ||
| } catch (e) { | ||
| console.error("[Task#updateClineMessage] Failed to flush message updates:", e) | ||
| } | ||
| }, this.MESSAGE_UPDATE_THROTTLE_MS) | ||
| } |
There was a problem hiding this comment.
The message update batching logic has a potential race condition. If updateClineMessage() is called rapidly with different messages, the timer could fire while new messages are still being added to the buffer, causing some messages to be sent in the next batch instead of the current one.
More critically, if the provider becomes unavailable or the panel becomes hidden between when messages are buffered and when the timer fires, those buffered messages will be lost since the early return check happens before the timer is set, not when it fires.
Consider moving the visibility check inside the timer callback to ensure buffered messages are either sent or explicitly dropped with logging.
…, atomic batch swap, drop/log deltas when hidden
There was a problem hiding this comment.
Pull Request Overview
This PR addresses performance issues in the webview by implementing throttling and batching mechanisms to prevent grey screens during long-running tasks. The changes reduce message volume and DOM churn while maintaining UX fidelity.
Key changes:
- Throttled state updates with coalescing in ClineProvider (~30 FPS)
- Dropped non-essential message updates when panel is hidden
- Batched chat message updates to prevent webview flooding
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/core/webview/ClineProvider.ts | Added throttling for state updates, indexing progress, and visibility-aware message filtering |
| src/core/task/Task.ts | Implemented batched message updates with buffering to reduce webview message flood |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Throttling/backpressure for heavy webview messages | ||
| private stateFlushQueued = false | ||
| private stateQueueDirty = false | ||
| private stateFlushPromise?: Promise<void> | ||
| private readonly STATE_THROTTLE_MS = 33 // ~30 FPS coalescing |
There was a problem hiding this comment.
[nitpick] Consider extracting these throttling constants to a configuration object or constants file to improve maintainability and make them easier to tune.
| // Allow any additional state requests queued during the await to be coalesced | ||
| if (this.stateQueueDirty) { | ||
| await delay(this.STATE_THROTTLE_MS) | ||
| } |
There was a problem hiding this comment.
The nested delay in the do-while loop could cause unnecessary delays. Consider checking if more requests arrived during the previous delay before adding another one.
| // Allow any additional state requests queued during the await to be coalesced | |
| if (this.stateQueueDirty) { | |
| await delay(this.STATE_THROTTLE_MS) | |
| } | |
| // No further delay here; immediately process any new requests | |
| // Loop will continue if this.stateQueueDirty was set during the above awaits |
src/core/task/Task.ts
Outdated
| } | ||
|
|
||
| // Batch UI updates within a short window to avoid overwhelming the webview | ||
| const ts = (message as any)?.ts as number | undefined |
There was a problem hiding this comment.
The type assertion (message as any) bypasses TypeScript's type safety. Consider adding a proper type guard or extending the ClineMessage interface to include the ts property if it's expected.
src/core/task/Task.ts
Outdated
|
|
||
| const providerNow = this.providerRef.deref() | ||
| if (!providerNow) { | ||
| console.warn( |
There was a problem hiding this comment.
[nitpick] Consider using the existing logging infrastructure (this.log or outputChannel) instead of console.warn for consistency with the rest of the codebase.
| console.warn( | |
| this.log.warn( |
| // Throttled chat row update batching to reduce webview message flood | ||
| private messageUpdateBuffer: Map<number, ClineMessage> = new Map() | ||
| private messageUpdateTimer?: NodeJS.Timeout | ||
| private readonly MESSAGE_UPDATE_THROTTLE_MS = 33 // ~30 FPS |
There was a problem hiding this comment.
The messageUpdateTimer should be cleared in the dispose() method to prevent the timer from firing after the task is disposed. Currently, if a task is disposed while a timer is pending, the timer callback will execute and attempt to access this.providerRef.deref() on a disposed task.
Consider adding this cleanup in the existing dispose() method (around line 1581):
if (this.messageUpdateTimer) {
clearTimeout(this.messageUpdateTimer)
this.messageUpdateTimer = undefined
}
| // Throttle noisy indexing status updates | ||
| private indexStatusThrottleTimer?: NodeJS.Timeout | ||
| private pendingIndexStatus?: IndexProgressUpdate | ||
| private readonly INDEX_STATUS_THROTTLE_MS = 100 |
There was a problem hiding this comment.
Similar to the messageUpdateTimer in Task.ts, the indexStatusThrottleTimer should be cleared when the provider is disposed to prevent the timer from firing after disposal.
Consider adding cleanup in the dispose() method or clearWebviewResources() method:
if (this.indexStatusThrottleTimer) {
clearTimeout(this.indexStatusThrottleTimer)
this.indexStatusThrottleTimer = undefined
}Task: sort batched messageUpdated by ts; clear messageUpdateTimer in dispose; use provider.log; remove any-cast for ts ClineProvider: void postMessage in throttle callbacks; clear indexStatusThrottleTimer in dispose
| const providerNow = this.providerRef.deref() | ||
| if (!providerNow) { | ||
| this.providerRef | ||
| .deref() | ||
| ?.log( | ||
| `[Task#updateClineMessage] Dropping ${batch.length} messageUpdated deltas: provider unavailable`, | ||
| ) | ||
| return | ||
| } | ||
| if (!providerNow.isVisible()) { | ||
| // Drop deltas while hidden; UI will receive a full state sync on visibility | ||
| this.providerRef | ||
| .deref() | ||
| ?.log( | ||
| `[Task#updateClineMessage] Dropping ${batch.length} messageUpdated deltas while hidden`, | ||
| ) | ||
| return |
There was a problem hiding this comment.
The logging calls on lines 691-695 and 700-705 will never execute successfully. When providerNow is undefined (line 690) or when checking visibility (line 698), the code calls this.providerRef.deref()?.log() which will return undefined again and skip the log via optional chaining.
For better observability, use the cached providerNow reference or fallback to console.log():
| const providerNow = this.providerRef.deref() | |
| if (!providerNow) { | |
| this.providerRef | |
| .deref() | |
| ?.log( | |
| `[Task#updateClineMessage] Dropping ${batch.length} messageUpdated deltas: provider unavailable`, | |
| ) | |
| return | |
| } | |
| if (!providerNow.isVisible()) { | |
| // Drop deltas while hidden; UI will receive a full state sync on visibility | |
| this.providerRef | |
| .deref() | |
| ?.log( | |
| `[Task#updateClineMessage] Dropping ${batch.length} messageUpdated deltas while hidden`, | |
| ) | |
| return | |
| const providerNow = this.providerRef.deref() | |
| if (!providerNow) { | |
| console.log( | |
| `[Task#updateClineMessage] Dropping ${batch.length} messageUpdated deltas: provider unavailable`, | |
| ) | |
| return | |
| } | |
| if (!providerNow.isVisible()) { | |
| providerNow.log( | |
| `[Task#updateClineMessage] Dropping ${batch.length} messageUpdated deltas while hidden`, | |
| ) | |
| return | |
| } |
hannesrudolph
left a comment
hannesrudolph
left a comment
There was a problem hiding this comment.
Implemented deterministic chat update ordering and safer throttles. Remaining suggestions are minor and tuning-oriented.
| // Throttled chat row update batching to reduce webview message flood | ||
| private messageUpdateBuffer: Map<number, ClineMessage> = new Map() | ||
| private messageUpdateTimer?: NodeJS.Timeout | ||
| private readonly MESSAGE_UPDATE_THROTTLE_MS = 33 // ~30 FPS |
There was a problem hiding this comment.
[P3] Consider centralizing throttle constants (MESSAGE_UPDATE_THROTTLE_MS, STATE_THROTTLE_MS, INDEX_STATUS_THROTTLE_MS) into a shared config to simplify tuning and keep defaults consistent across Task and ClineProvider.
| await this.postMessageToWebview({ type: "state", state }) | ||
|
|
||
| // Check MDM compliance and send user to account tab if not compliant | ||
| // Only redirect if there's an actual MDM policy requiring authentication |
There was a problem hiding this comment.
[P3] The extra delay inside flushStateQueue's loop adds latency under sustained load. Consider removing the inner delay and relying on the loop’s coalescing to maintain responsiveness without materially increasing message volume.
…x: guard dev FS watchers in activate() and MCP watchers in tests; core(task): safe provider logging in batched webview updates
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
|
Does not fix the issue in the way I have reproed it. |
fix(webview): throttle state/message updates to prevent grey screens during long tasks
Summary
Changes
Why
Verification
Risk/Notes
Important
Optimize webview performance by throttling state and message updates, reducing update frequency during long tasks.
ClineProvider.postStateToWebview()to coalesce state updates (~30 FPS).messageUpdateddeltas when panel is hidden; push full state sync on visibility.ClineProvider.messageUpdateddeltas to ~30 FPS inTask.updateClineMessage(); skip when hidden.RelativePatternmock invscode.js.extension.tsto conditionally watch files in development.isTestEnvironment()helper inMcpHub.ts.This description was created by
for 62557dc. You can customize this summary. It will automatically update as commits are pushed.