-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix(webview): throttle state/message updates to prevent grey screens during long tasks #8609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
1f7e78e
03d28f9
458aac2
62557dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -277,6 +277,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |||||
| public readonly messageQueueService: MessageQueueService | ||||||
| private messageQueueStateChangedHandler: (() => void) | undefined | ||||||
|
|
||||||
| // 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 | ||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||||||
|
|
||||||
| // Streaming | ||||||
| isWaitingForFirstChunk = false | ||||||
| isStreaming = false | ||||||
|
|
@@ -644,17 +649,66 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |||||
|
|
||||||
| private async updateClineMessage(message: ClineMessage) { | ||||||
| const provider = this.providerRef.deref() | ||||||
| await provider?.postMessageToWebview({ type: "messageUpdated", clineMessage: message }) | ||||||
|
|
||||||
| // Emit internal event immediately (used for token usage, etc) | ||||||
| this.emit(RooCodeEventName.Message, { action: "updated", message }) | ||||||
|
|
||||||
| // Telemetry capture remains unchanged below | ||||||
| const shouldCaptureMessage = message.partial !== true && CloudService.isEnabled() | ||||||
|
|
||||||
| if (shouldCaptureMessage) { | ||||||
| CloudService.instance.captureEvent({ | ||||||
| event: TelemetryEventName.TASK_MESSAGE, | ||||||
| properties: { taskId: this.taskId, message }, | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| // Batch UI updates within a short window to avoid overwhelming the webview | ||||||
| const ts = (message as any)?.ts as number | undefined | ||||||
|
||||||
| if (typeof ts === "number") { | ||||||
| this.messageUpdateBuffer.set(ts, message) | ||||||
| } else { | ||||||
| // Fallback: no timestamp - only send if panel is visible and provider exists | ||||||
| if (!provider || !provider.isVisible()) { | ||||||
| // Drop non-essential delta; UI will resync on next visibility/state push | ||||||
| return | ||||||
| } | ||||||
| await provider.postMessageToWebview({ type: "messageUpdated", clineMessage: message }) | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| if (!this.messageUpdateTimer) { | ||||||
| this.messageUpdateTimer = setTimeout(async () => { | ||||||
| try { | ||||||
| // Atomically swap buffers so new arrivals during flush aren't lost | ||||||
| const batchMap = this.messageUpdateBuffer | ||||||
| this.messageUpdateBuffer = new Map() | ||||||
| this.messageUpdateTimer = undefined | ||||||
|
|
||||||
| const batch = Array.from(batchMap.values()) | ||||||
|
|
||||||
| const providerNow = this.providerRef.deref() | ||||||
| if (!providerNow) { | ||||||
| console.warn( | ||||||
|
||||||
| console.warn( | |
| this.log.warn( |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -148,6 +148,17 @@ export class ClineProvider | |||||||||||||
| public readonly providerSettingsManager: ProviderSettingsManager | ||||||||||||||
| public readonly customModesManager: CustomModesManager | ||||||||||||||
|
|
||||||||||||||
| // 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 | ||||||||||||||
|
Comment on lines
+151
to
+155
|
||||||||||||||
|
|
||||||||||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the Consider adding cleanup in the if (this.indexStatusThrottleTimer) {
clearTimeout(this.indexStatusThrottleTimer)
this.indexStatusThrottleTimer = undefined
} |
||||||||||||||
|
|
||||||||||||||
| constructor( | ||||||||||||||
| readonly context: vscode.ExtensionContext, | ||||||||||||||
| private readonly outputChannel: vscode.OutputChannel, | ||||||||||||||
|
|
@@ -799,6 +810,8 @@ export class ClineProvider | |||||||||||||
| const viewStateDisposable = webviewView.onDidChangeViewState(() => { | ||||||||||||||
| if (this.view?.visible) { | ||||||||||||||
| this.postMessageToWebview({ type: "action", action: "didBecomeVisible" }) | ||||||||||||||
| // Push latest state when tab becomes visible to avoid stale UI without flooding while hidden | ||||||||||||||
| void this.postStateToWebview() | ||||||||||||||
| } | ||||||||||||||
| }) | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -808,6 +821,8 @@ export class ClineProvider | |||||||||||||
| const visibilityDisposable = webviewView.onDidChangeVisibility(() => { | ||||||||||||||
| if (this.view?.visible) { | ||||||||||||||
| this.postMessageToWebview({ type: "action", action: "didBecomeVisible" }) | ||||||||||||||
| // Ensure UI sync after becoming visible | ||||||||||||||
| void this.postStateToWebview() | ||||||||||||||
| } | ||||||||||||||
| }) | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -971,7 +986,16 @@ export class ClineProvider | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public async postMessageToWebview(message: ExtensionMessage) { | ||||||||||||||
| await this.view?.webview.postMessage(message) | ||||||||||||||
| try { | ||||||||||||||
| // Reduce background chatter: drop chat row updates when panel isn't visible | ||||||||||||||
| if (message.type === "messageUpdated" && !this.isVisible()) { | ||||||||||||||
| return | ||||||||||||||
| } | ||||||||||||||
| if (!this.view) return | ||||||||||||||
| await this.view.webview.postMessage(message) | ||||||||||||||
| } catch (error) { | ||||||||||||||
| this.log(`[postMessageToWebview] failed: ${error instanceof Error ? error.message : String(error)}`) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private async getHMRHtmlContent(webview: vscode.Webview): Promise<string> { | ||||||||||||||
|
|
@@ -1602,13 +1626,40 @@ export class ClineProvider | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| async postStateToWebview() { | ||||||||||||||
| const state = await this.getStateToPostToWebview() | ||||||||||||||
| this.postMessageToWebview({ type: "state", state }) | ||||||||||||||
| // Coalesce multiple rapid callers into a single flush, awaited by all | ||||||||||||||
| this.stateQueueDirty = true | ||||||||||||||
| if (!this.stateFlushQueued) { | ||||||||||||||
| this.stateFlushQueued = true | ||||||||||||||
| this.stateFlushPromise = this.flushStateQueue() | ||||||||||||||
| } | ||||||||||||||
| return this.stateFlushPromise | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private async flushStateQueue(): Promise<void> { | ||||||||||||||
| try { | ||||||||||||||
| // Small delay window to coalesce bursts from multiple sources | ||||||||||||||
| await delay(this.STATE_THROTTLE_MS) | ||||||||||||||
|
|
||||||||||||||
| // Drain while new calls arrive during awaits | ||||||||||||||
| do { | ||||||||||||||
| this.stateQueueDirty = false | ||||||||||||||
|
|
||||||||||||||
| // Check MDM compliance and send user to account tab if not compliant | ||||||||||||||
| // Only redirect if there's an actual MDM policy requiring authentication | ||||||||||||||
| if (this.mdmService?.requiresCloudAuth() && !this.checkMdmCompliance()) { | ||||||||||||||
| await this.postMessageToWebview({ type: "action", action: "cloudButtonClicked" }) | ||||||||||||||
| const state = await this.getStateToPostToWebview() | ||||||||||||||
| 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 | ||||||||||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||||||||||||||
| if (this.mdmService?.requiresCloudAuth() && !this.checkMdmCompliance()) { | ||||||||||||||
| await this.postMessageToWebview({ type: "action", action: "cloudButtonClicked" }) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Allow any additional state requests queued during the await to be coalesced | ||||||||||||||
| if (this.stateQueueDirty) { | ||||||||||||||
| await delay(this.STATE_THROTTLE_MS) | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+1663
to
+1666
|
||||||||||||||
| // 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
messageUpdateTimershould be cleared in thedispose()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 accessthis.providerRef.deref()on a disposed task.Consider adding this cleanup in the existing
dispose()method (around line 1581):