fix(msteams): update FileConsentCard after user accepts upload#49580
fix(msteams): update FileConsentCard after user accepts upload#49580sudie-codes wants to merge 2 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes the stale FileConsentCard UX in MS Teams by (1) storing the consent card's activity ID in Key concerns:
Confidence Score: 3/5
|
There was a problem hiding this comment.
Core FileConsentCard fix is correct, tracking consentCardActivityId in PendingUpload and calling updateActivity() after upload success is the right approach. Non-fatal handling (catch around updateActivity) is appropriate. Timer cleanup with clearTimeout() on early removal is a good catch.
Key concern: This PR overlaps heavily with #49582 (security fix), #49585 (Graph chat ID), and #49587 (singleton guard), ~400 lines of shared code. Specifically:
resolveGraphChatIdhere is less safe than #49585's version (returns first result without ambiguity check vs. returningnullon ambiguous matches)- Recommend using #49585's implementation as the canonical one
Merge order: This should land last in the series (#49582 → #49929 → #49585 → #49925 → #49587 → #49580) and will need a rebase to drop the duplicated code.
Minor nits: double-if guard in monitor could be if/else, and one error log is missing the error object.
✅ Approve, with rebase after the other PRs land.
2d78fac to
62d1967
Compare
62d1967 to
6578951
Compare
|
@sudie-codes Heads up — both #49925 and this PR need rebases against For #49925: there are merge conflicts and 2 failing CI checks ( For this PR (#49580): once #49925 is rebased and merged, you'll need to rebase this one too. The |
There was a problem hiding this comment.
Review: fix(msteams) — update FileConsentCard after user accepts upload
Verdict: Request changes 🔴 (minor — one missing test)
The intent is right — replacing the stale "pending" consent card with the completed file info card is a real UX improvement. Implementation logic is correct.
What works well
consentCardActivityIdtracking inpending-uploads.tsis clean. Storing the activity ID at consent-card-send time and retrieving it at upload-complete time is the right approach.- Non-fatal
updateActivityfallback — if the card replacement fails, fall back to sending a new message. Good defensive pattern for cases where the bot loses edit permission or the activity ID goes stale. - Dual guard structure in
monitor-handler.tsis correct:if (!consentCardActivityId)sends a new message;if (consentCardActivityId)replaces the consent card in-place. These are complementary guards, not a dual-send bug. - Pending upload test coverage is thorough — TTL expiry, timer cleanup on explicit removal,
clearPendingUploads, and edge cases (undefined/unknown IDs).
Missing test (blocking)
Add a test asserting: when consentCardActivityId is present and updateActivity succeeds, sendActivity is not called for the file info card.
The current test suite covers the updateActivity failure fallback path (which correctly sends a new message), but does not assert the happy path avoids a duplicate send. This is the regression guard that ensures future refactors do not accidentally introduce double cards.
CI failures
Not caused by this PR. The extensions test failure is the pre-existing setup-surface.test.ts async assertion issue.
Bottom line
Logic is sound, coverage is close. Add the happy-path no-duplicate-send assertion and this is good to go.
BradGroux
left a comment
There was a problem hiding this comment.
Review: FileConsentCard Update After Upload
Summary
The PR adds a consentCardActivityId field to PendingUpload and uses it in the accept handler to update the original consent card after a successful upload. The timer cleanup improvements (clearing the timeout on accept/decline) are solid and prevent stale state.
Blocker: consentCardActivityId is never populated in production
The core issue is a data flow gap. The storePendingUpload() call in file-consent-helpers.ts (the real send path) does not capture or store the activity ID returned from sending the consent card. The field exists in the type, and the accept handler reads it, but nothing in the actual production flow ever writes it.
The tests pass because they inject consentCardActivityId directly into the pending upload fixture. This masks the fact that in a real bot conversation, the field will always be undefined, and the updateActivity() call will silently skip (or fail).
To fix: Capture the activity ID from the sendActivity() response when sending the consent card, and pass it through to storePendingUpload().
What works well
- Timer cleanup on accept/decline prevents orphaned timeouts
- Test coverage is thorough for the happy path
- Type additions are clean
Verdict
Request Changes. The timer fix is worth merging on its own, but the headline feature (updating the consent card) cannot work without wiring the activity ID from the send path into storage.
|
@BradGroux Thanks for the thorough review — both issues addressed in Blocker (consentCardActivityId never populated): Added Missing test: Added a happy-path test asserting that when All 258 msteams tests pass. |
There was a problem hiding this comment.
Review: fix(msteams) FileConsentCard update after user accepts upload
Commit reviewed: bf45487
Blocker from prior review: RESOLVED ✅
The original review flagged that consentCardActivityId was never populated in the real send path, making the in-place card update dead code. This is now fixed end-to-end:
buildActivity()tags the consent activity with_pendingUploadIdin the activity valuesendMSTeamsMessages()extracts the tag, strips it before sending over the wire, and callssetPendingUploadActivityId(uploadId, messageId)to persist the mappingsend.ts(proactive path) also callssetPendingUploadActivityIdaftersendActivitymonitor-handler.tsretrieves the storedconsentCardActivityIdand uses it withupdateActivityto replace the consent card in-place, with a fallback tosendActivityif the update fails
The new setPendingUploadActivityId() function in pending-uploads.ts cleanly encapsulates the persistence logic.
Test coverage
- ✅ Happy path test: asserts
updateActivitycalled once,sendActivityNOT called with file info card - ✅ Fallback test: when
updateActivitythrows, verifiessendActivityis used instead
Suggestions (non-blocking)
1. Guard setPendingUploadActivityId calls (Low)
Consider wrapping setPendingUploadActivityId(...) in a try/catch with logging. If the persistence layer ever changes or throws unexpectedly, the upload flow should not break just because activity ID tracking failed. The upload itself is the critical path; the in-place card update is a UX enhancement.
2. Telemetry on updateActivity fallback (Low)
When updateActivity fails and the fallback sendActivity path is used, consider emitting a log or telemetry event. This helps diagnose Teams-side issues (expired activity IDs, permission changes) without needing to reproduce the failure.
3. Duplicate consent card scenario (Low)
If retries or race conditions cause multiple consent cards to be sent for the same upload ID, only the last setPendingUploadActivityId call wins. The earlier consent card(s) become orphaned in the conversation. This is an edge case but worth considering whether a cleanup mechanism or dedupe check would be valuable.
4. Race condition: accept before persistence (Low)
If the user clicks "Accept" on the consent card before setPendingUploadActivityId has completed, the consentCardActivityId lookup returns nothing and the fallback (send new activity) is used. This degrades gracefully, but is unverified by tests. A test asserting the fallback behavior when consentCardActivityId is missing would strengthen confidence.
Summary
This PR correctly fixes the FileConsentCard update flow. The consentCardActivityId is now wired through both the messenger and proactive send paths. The fallback behavior on update failure is clean. All suggestions above are non-blocking improvements.
|
This has merge conflicts against |
- Adds consentCardActivityId to PendingUpload so the consent card activity can be replaced in-place after upload succeeds - Uses context.updateActivity() to replace the FileConsentCard with the file info card; falls back to sendActivity if update fails - Adds updateActivity to MSTeamsTurnContext type - Fixes timer leak in pending-uploads: clears TTL setTimeout on explicit removal and on clearPendingUploads() - Adds pending-uploads.test.ts covering all new timer/cleanup paths
…th updateActivity test Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
bf45487 to
c50db13
Compare
|
@BradGroux Rebased onto
All 459 msteams tests pass. |
Summary
Fixes #47268 — After a user accepts a
FileConsentCardin MS Teams, the original consent card remained in its "pending" state instead of being replaced with the completed file info card.What & Why
Problem: When the bot sends a file to a 1:1 DM user via
FileConsentCard, the user clicks "Accept", the file uploads successfully, and a new file info card is sent — but the original consent card stays visible in the chat showing "pending". Users don't know if the upload succeeded, leading to confusion and repeated attempts.Root cause:
monitor-handler.tssent the file info card viacontext.sendActivity()but never calledcontext.updateActivity()to replace/remove the original consent card. Additionally, the consent card's activity ID was not being tracked alongside the pending upload metadata.Fix:
pending-uploads.ts: AddedconsentCardActivityIdfield toPendingUploadtype; fixed timer cleanup gap —removePendingUploadnow callsclearTimeout()on the TTL timer so ghost Map deletions can't fire after early removalmonitor-handler.ts: After successful upload + file info card send, callscontext.updateActivity()to replace the original consent card (non-fatal — upload success is not blocked if the card update fails)sdk-types.ts: AddedupdateActivitytoMSTeamsTurnContexttypeFiles changed:
extensions/msteams/src/pending-uploads.ts,extensions/msteams/src/monitor-handler.ts,extensions/msteams/src/sdk-types.tsScreenshots
N/A — This is a server-side bot framework fix. The change affects how Bot Framework
updateActivity()is called after file upload completion. Verification requires a running Azure Bot Service instance connected to a Teams client. The fix is validated via unit tests that mock the turn context.Test Results
updateActivitytest cases inmonitor-handler.file-consent.test.tspending-uploads.test.tsAI Disclosure
updateActivity()replaces the stale consent card; timer refs are tracked and cleared on early removal to prevent memory leaks