-
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 all 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 | ||
|
|
@@ -298,6 +303,17 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| private tokenUsageSnapshot?: TokenUsage | ||
| private tokenUsageSnapshotAt?: number | ||
|
|
||
| private safeProviderLog(message: string): void { | ||
| try { | ||
| const provider = this.providerRef.deref() | ||
| if (provider && typeof (provider as any).log === "function") { | ||
| ;(provider as any).log(message) | ||
| } | ||
| } catch { | ||
| // no-op (tests/mocks may not implement log) | ||
| } | ||
| } | ||
|
|
||
| constructor({ | ||
| provider, | ||
| apiConfiguration, | ||
|
|
@@ -461,7 +477,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| this._taskMode = defaultModeSlug | ||
| // Use the provider's log method for better error visibility | ||
| const errorMessage = `Failed to initialize task mode: ${error instanceof Error ? error.message : String(error)}` | ||
| provider.log(errorMessage) | ||
| this.safeProviderLog(errorMessage) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -644,17 +660,70 @@ 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.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()).sort((a, b) => (a.ts ?? 0) - (b.ts ?? 0)) | ||
|
|
||
| const providerNow = this.providerRef.deref() | ||
| if (!providerNow) { | ||
| this.safeProviderLog( | ||
| `[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.safeProviderLog( | ||
| `[Task#updateClineMessage] Dropping ${batch.length} messageUpdated deltas while hidden`, | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| for (const m of batch) { | ||
| await providerNow.postMessageToWebview({ type: "messageUpdated", clineMessage: m }) | ||
| } | ||
| } catch (e) { | ||
| this.safeProviderLog( | ||
| `[Task#updateClineMessage] Failed to flush message updates: ${ | ||
| e instanceof Error ? e.message : String(e) | ||
| }`, | ||
| ) | ||
| } | ||
| }, this.MESSAGE_UPDATE_THROTTLE_MS) | ||
| } | ||
| } | ||
|
|
||
| private async saveClineMessages() { | ||
|
|
@@ -1527,6 +1596,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| public dispose(): void { | ||
| console.log(`[Task#dispose] disposing task ${this.taskId}.${this.instanceId}`) | ||
|
|
||
| // Clear pending batched message timer and buffer | ||
| if (this.messageUpdateTimer) { | ||
| clearTimeout(this.messageUpdateTimer) | ||
| this.messageUpdateTimer = undefined | ||
| } | ||
| this.messageUpdateBuffer.clear() | ||
|
|
||
| // Dispose message queue and remove event listeners. | ||
| try { | ||
| if (this.messageQueueStateChangedHandler) { | ||
|
|
@@ -1670,9 +1746,9 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| // including the subtask result, not just from before the subtask was created | ||
| this.skipPrevResponseIdOnce = true | ||
| } catch (error) { | ||
| this.providerRef | ||
| .deref() | ||
| ?.log(`Error failed to add reply from subtask into conversation of parent task, error: ${error}`) | ||
| this.safeProviderLog( | ||
| `Error failed to add reply from subtask into conversation of parent task, error: ${error}`, | ||
| ) | ||
|
|
||
| throw error | ||
| } | ||
|
|
@@ -1763,9 +1839,9 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| const provider = this.providerRef.deref() | ||
|
|
||
| if (this.isPaused && provider) { | ||
| provider.log(`[subtasks] paused ${this.taskId}.${this.instanceId}`) | ||
| this.safeProviderLog(`[subtasks] paused ${this.taskId}.${this.instanceId}`) | ||
| await this.waitForSubtask() | ||
| provider.log(`[subtasks] resumed ${this.taskId}.${this.instanceId}`) | ||
| this.safeProviderLog(`[subtasks] resumed ${this.taskId}.${this.instanceId}`) | ||
| const currentMode = (await provider.getState())?.mode ?? defaultModeSlug | ||
|
|
||
| if (currentMode !== this.pausedModeSlug) { | ||
|
|
@@ -1775,7 +1851,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| // Delay to allow mode change to take effect before next tool is executed. | ||
| await delay(500) | ||
|
|
||
| provider.log( | ||
| this.safeProviderLog( | ||
| `[subtasks] task ${this.taskId}.${this.instanceId} has switched back to '${this.pausedModeSlug}' from '${currentMode}'`, | ||
| ) | ||
| } | ||
|
|
||
| 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, | ||||||||||||||
|
|
@@ -572,6 +583,13 @@ export class ClineProvider | |||||||||||||
| async dispose() { | ||||||||||||||
| this.log("Disposing ClineProvider...") | ||||||||||||||
|
|
||||||||||||||
| // Clear throttled timers | ||||||||||||||
| if (this.indexStatusThrottleTimer) { | ||||||||||||||
| clearTimeout(this.indexStatusThrottleTimer) | ||||||||||||||
| this.indexStatusThrottleTimer = undefined | ||||||||||||||
| } | ||||||||||||||
| this.pendingIndexStatus = undefined | ||||||||||||||
|
|
||||||||||||||
| // Clear all tasks from the stack. | ||||||||||||||
| while (this.clineStack.length > 0) { | ||||||||||||||
| await this.removeClineFromStack() | ||||||||||||||
|
|
@@ -799,6 +817,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 +828,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 +993,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 +1633,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) | ||||||||||||||
|
|
||||||||||||||
| // 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" }) | ||||||||||||||
| // Drain while new calls arrive during awaits | ||||||||||||||
| do { | ||||||||||||||
| this.stateQueueDirty = false | ||||||||||||||
|
|
||||||||||||||
| 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):