Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 40 additions & 1 deletion src/core/task/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -644,9 +649,12 @@ 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) {
Expand All @@ -655,6 +663,37 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
properties: { taskId: this.taskId, message },
})
}

// If provider is unavailable, or panel is hidden, skip UI delta updates.
// The UI will resync on next visibility or full state push.
if (!provider || (typeof (provider as any).isVisible === "function" && !(provider as any).isVisible())) {
return
}

// Batch UI updates within a short window to avoid overwhelming the webview
const ts = (message as any)?.ts as number | undefined
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
if (typeof ts === "number") {
this.messageUpdateBuffer.set(ts, message)
} else {
// Fallback: no timestamp, just send immediately
await provider.postMessageToWebview({ type: "messageUpdated", clineMessage: message })
return
}

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)
}
Comment on lines 690 to 726
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

private async saveClineMessages() {
Expand Down
95 changes: 81 additions & 14 deletions src/core/webview/ClineProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider extracting these throttling constants to a configuration object or constants file to improve maintainability and make them easier to tune.

Copilot uses AI. Check for mistakes.

// Throttle noisy indexing status updates
private indexStatusThrottleTimer?: NodeJS.Timeout
private pendingIndexStatus?: IndexProgressUpdate
private readonly INDEX_STATUS_THROTTLE_MS = 100
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}


constructor(
readonly context: vscode.ExtensionContext,
private readonly outputChannel: vscode.OutputChannel,
Expand Down Expand Up @@ -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()
}
})

Expand All @@ -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()
}
})

Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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

Copilot uses AI. Check for mistakes.
} while (this.stateQueueDirty)
} finally {
this.stateFlushQueued = false
}
}

Expand Down Expand Up @@ -2288,6 +2339,11 @@ export class ClineProvider
return this._workspaceTracker
}

// Public visibility helper for rate limiting background chatter
public isVisible(): boolean {
return this.view?.visible === true
}

get viewLaunched() {
return this.isViewLaunched
}
Expand Down Expand Up @@ -2409,13 +2465,24 @@ export class ClineProvider
if (currentManager) {
this.codeIndexStatusSubscription = currentManager.onProgressUpdate((update: IndexProgressUpdate) => {
// Only send updates if this manager is still the current one
if (currentManager === this.getCurrentWorkspaceCodeIndexManager()) {
// Get the full status from the manager to ensure we have all fields correctly formatted
const fullStatus = currentManager.getCurrentStatus()
this.postMessageToWebview({
type: "indexingStatusUpdate",
values: fullStatus,
})
if (currentManager !== this.getCurrentWorkspaceCodeIndexManager()) {
return
}

// Throttle: coalesce frequent progress updates into ~10 Hz
this.pendingIndexStatus = currentManager.getCurrentStatus()

if (!this.indexStatusThrottleTimer) {
this.indexStatusThrottleTimer = setTimeout(() => {
const values = this.pendingIndexStatus ?? currentManager.getCurrentStatus()
this.pendingIndexStatus = undefined
this.indexStatusThrottleTimer = undefined

this.postMessageToWebview({
type: "indexingStatusUpdate",
values,
})
}, this.INDEX_STATUS_THROTTLE_MS)
}
})

Expand Down