fix(msteams): keep streaming alive during long tool chains via typing indicator (#59731)#63963
Conversation
|
Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit. |
Greptile SummaryThis PR fixes dropped bot replies in Teams during long tool chains (30s+) by sending periodic Confidence Score: 5/5Safe to merge; all remaining findings are minor style/documentation suggestions. The fix is well-scoped and the new No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/msteams/src/reply-dispatcher.ts
Line: 257-261
Comment:
**Stale "supported by this conversation type" wording**
The comment refers to gating on conversation type support, but the condition below it was changed from `isTypingSupported && typingIndicatorEnabled` to just `typingIndicatorEnabled`. Channel conversations now also start the keepalive loop (even though `sendTypingIndicator` is a no-op there). The prose should reflect the new intent.
```suggestion
// Always start the typing keepalive loop when typing is enabled.
// The sendTypingIndicator gate skips actual sends while the stream
// card is visually active, so during the first text segment the user
// only sees the streaming UI.
// Once the stream finalizes (between segments / during tool chains),
// the loop starts sending typing activities and keeps the Bot Framework
// TurnContext alive so the post-tool reply can still land. See #59731.
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/msteams/src/reply-stream-controller.ts
Line: 137-145
Comment:
**`isFailed` not checked in `isStreamActive`**
`isStreamActive` guards against `isFinalized` but not `isFailed`. In `TeamsHttpStream`, `streamFailed = true` always triggers an immediate `void this.finalize()` call, so `isFinalized` becomes `true` before any external caller can observe `isFailed` in isolation. This means the omission is safe today, but the JSDoc lists only "finalized" scenarios and doesn't mention the failure case, making the invariant implicit. A defensive check or an explicit doc note would make the reasoning self-contained.
```suggestion
isStreamActive(): boolean {
if (!stream) {
return false;
}
if (stream.isFinalized || stream.isFailed) {
return false;
}
return streamReceivedTokens;
},
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(msteams): keep streaming alive durin..." | Re-trigger Greptile |
| await streamController.onReplyStart(); | ||
| // Avoid duplicate typing UX in DMs: stream status already shows progress. | ||
| if (typingIndicatorEnabled && !streamController.hasStream()) { | ||
| // Always start the typing keepalive loop when typing is enabled and | ||
| // supported by this conversation type. The sendTypingIndicator gate | ||
| // skips actual sends while the stream card is visually active, so | ||
| // during the first text segment the user only sees the streaming UI. |
There was a problem hiding this comment.
The comment refers to gating on conversation type support, but the condition below it was changed from isTypingSupported && typingIndicatorEnabled to just typingIndicatorEnabled. Channel conversations now also start the keepalive loop (even though sendTypingIndicator is a no-op there). The prose should reflect the new intent.
| await streamController.onReplyStart(); | |
| // Avoid duplicate typing UX in DMs: stream status already shows progress. | |
| if (typingIndicatorEnabled && !streamController.hasStream()) { | |
| // Always start the typing keepalive loop when typing is enabled and | |
| // supported by this conversation type. The sendTypingIndicator gate | |
| // skips actual sends while the stream card is visually active, so | |
| // during the first text segment the user only sees the streaming UI. | |
| // Always start the typing keepalive loop when typing is enabled. | |
| // The sendTypingIndicator gate skips actual sends while the stream | |
| // card is visually active, so during the first text segment the user | |
| // only sees the streaming UI. | |
| // Once the stream finalizes (between segments / during tool chains), | |
| // the loop starts sending typing activities and keeps the Bot Framework | |
| // TurnContext alive so the post-tool reply can still land. See #59731. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/reply-dispatcher.ts
Line: 257-261
Comment:
**Stale "supported by this conversation type" wording**
The comment refers to gating on conversation type support, but the condition below it was changed from `isTypingSupported && typingIndicatorEnabled` to just `typingIndicatorEnabled`. Channel conversations now also start the keepalive loop (even though `sendTypingIndicator` is a no-op there). The prose should reflect the new intent.
```suggestion
// Always start the typing keepalive loop when typing is enabled.
// The sendTypingIndicator gate skips actual sends while the stream
// card is visually active, so during the first text segment the user
// only sees the streaming UI.
// Once the stream finalizes (between segments / during tool chains),
// the loop starts sending typing activities and keeps the Bot Framework
// TurnContext alive so the post-tool reply can still land. See #59731.
```
How can I resolve this? If you propose a fix, please make it concise.| isStreamActive(): boolean { | ||
| if (!stream) { | ||
| return false; | ||
| } | ||
| if (stream.isFinalized) { | ||
| return false; | ||
| } | ||
| return streamReceivedTokens; | ||
| }, |
There was a problem hiding this comment.
isFailed not checked in isStreamActive
isStreamActive guards against isFinalized but not isFailed. In TeamsHttpStream, streamFailed = true always triggers an immediate void this.finalize() call, so isFinalized becomes true before any external caller can observe isFailed in isolation. This means the omission is safe today, but the JSDoc lists only "finalized" scenarios and doesn't mention the failure case, making the invariant implicit. A defensive check or an explicit doc note would make the reasoning self-contained.
| isStreamActive(): boolean { | |
| if (!stream) { | |
| return false; | |
| } | |
| if (stream.isFinalized) { | |
| return false; | |
| } | |
| return streamReceivedTokens; | |
| }, | |
| isStreamActive(): boolean { | |
| if (!stream) { | |
| return false; | |
| } | |
| if (stream.isFinalized || stream.isFailed) { | |
| return false; | |
| } | |
| return streamReceivedTokens; | |
| }, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/reply-stream-controller.ts
Line: 137-145
Comment:
**`isFailed` not checked in `isStreamActive`**
`isStreamActive` guards against `isFinalized` but not `isFailed`. In `TeamsHttpStream`, `streamFailed = true` always triggers an immediate `void this.finalize()` call, so `isFinalized` becomes `true` before any external caller can observe `isFailed` in isolation. This means the omission is safe today, but the JSDoc lists only "finalized" scenarios and doesn't mention the failure case, making the invariant implicit. A defensive check or an explicit doc note would make the reasoning self-contained.
```suggestion
isStreamActive(): boolean {
if (!stream) {
return false;
}
if (stream.isFinalized || stream.isFailed) {
return false;
}
return streamReceivedTokens;
},
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0e1ed63f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (stream.isFinalized) { | ||
| return false; | ||
| } | ||
| return streamReceivedTokens; |
There was a problem hiding this comment.
Gate keepalive on actual stream sends, not token receipt
isStreamActive() currently returns streamReceivedTokens, which flips to true on any onPartialReply text even when Teams has not emitted a streaming chunk yet (for example, text shorter than the MIN_INITIAL_CHARS threshold in streaming-message.ts). In that state sendTypingIndicator is suppressed, so if the model emits a short pre-tool prefix and then runs tools for 30s+, no activity is sent and the TurnContext can still expire—the timeout this change is trying to prevent.
Useful? React with 👍 / 👎.
Summary
Bot replies in Teams no longer get dropped mid-stream when the agent runs a long tool chain (30s+). The plugin now sends periodic typing indicators during tool execution to keep the conversation visually active and prevent connector timeouts.
What changed
Test plan
Fixes #59731