MM-67409 Adding support for transcripts, recording, AI summarization and meeting subscription to channels#455
Conversation
…ng-and-ai-summarization
…ng-and-ai-summarization
…ng-and-ai-summarization
…ng-and-ai-summarization
…ng-and-ai-summarization
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
server/plugin_test.go (2)
173-174: Consider tightening these optional KV mocks.The broad
HasPrefix(...).Maybe()matchers can mask unintended KV interactions. Prefer scenario-specific expectations (or explicit call counts) where possible to keep regression detection sharp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin_test.go` around lines 173 - 174, The KV mocks for KVGet and KVSetWithExpiry are too permissive—replace the broad mock.MatchedBy(func(key string) bool { return strings.HasPrefix(key, meetingChannelKey) }).Maybe() with tighter expectations: assert the exact expected key(s) or use mock.MatchedBy predicates that validate the full constructed key (including room/meeting IDs) and set explicit call counts (e.g., Once() or Times(n)) instead of Maybe(); update the KVSetWithExpiry matcher to check the byte payload/type (instead of mock.AnythingOfType) and the exact TTL constant adHocMeetingChannelTTL so tests fail on unexpected KV interactions and detect regressions involving KVGet/KVSetWithExpiry.
68-71: Add at least one test case with a realistic Zoom UUID shape.Using
"uuid":"234"won’t catch regressions around UUID normalization/lookup. Please add a case with special characters (common in Zoom UUIDs), so webhook-to-KV mapping is actually stress-tested.🧪 Example test payload shape
- endedPayload := `{"event": "meeting.ended", "payload": {"object": {"id": "234", "uuid": "234"}}}` + endedPayload := `{"event": "meeting.ended", "payload": {"object": {"id": "234", "uuid": "/nk84I4+Q3a7zA=="}}}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin_test.go` around lines 68 - 71, The tests use trivial UUIDs ("234") which won't catch normalization/lookup bugs; update the test data around endedPayload, validStoppedWebhookRequest and validStartedWebhookRequest to include at least one realistic Zoom-style UUID containing special characters (e.g., characters like /, +, =, or underscores) and add assertions or an additional test request that uses that UUID shape so webhook-to-KV mapping logic is exercised; locate and modify the variables endedPayload, validStoppedWebhookRequest and validStartedWebhookRequest in the test to include the new payload string(s).server/webhook_test.go (1)
381-459: Add explicit HTTP status assertions in the new webhook handler tests.These tests currently rely on mock expectations only. Adding response status checks makes failures easier to diagnose and catches response-path regressions.
💡 Suggested test hardening
func TestWebhookHandleTranscriptCompleted(t *testing.T) { @@ p.ServeHTTP(&plugin.Context{}, w, request) body, _ := io.ReadAll(w.Result().Body) t.Log(string(body)) + require.Equal(t, http.StatusOK, w.Result().StatusCode) api.AssertExpectations(t) } @@ func TestWebhookHandleRecordingCompleted(t *testing.T) { @@ p.ServeHTTP(&plugin.Context{}, w, request) body, _ := io.ReadAll(w.Result().Body) t.Log(string(body)) + require.Equal(t, http.StatusOK, w.Result().StatusCode) api.AssertExpectations(t) }Also applies to: 461-531
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/webhook_test.go` around lines 381 - 459, The test TestWebhookHandleTranscriptCompleted is missing explicit HTTP status assertions; after calling p.ServeHTTP(&plugin.Context{}, w, request) and reading the response body, assert the expected HTTP status (e.g., 200 OK or the specific code your handler should return) using w.Result().StatusCode (and optionally check w.Result().Status) so failures surface in test output; apply the same pattern to the other webhook tests referenced (lines 461-531) to harden them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin.json`:
- Around line 107-113: Update the setting for key
"EnablePostingRecordingPassword" so its help_text includes an explicit security
warning: state that enabling will post recording credentials to the channel,
making them visible to all channel members and stored in channel
history/exports, and advise admins to only enable for trusted channels or use
alternate secure distribution; modify the help_text value for the
"EnablePostingRecordingPassword" entry accordingly (preserve other fields like
default/type).
In `@server/store.go`:
- Around line 371-373: The code silently ignores JSON unmarshal errors for the
subscription index (raw -> idx); update the block where json.Unmarshal(raw,
&idx) is called to capture and handle the error instead of discarding it: check
the error returned by json.Unmarshal, and on error log a clear message including
the error and the offending payload identifier, then either return the error
(propagate) or remove/repair the corrupted entry so callers don't get an
empty/incomplete subscription list; refer to the variables raw and idx and the
json.Unmarshal call to locate the change.
- Around line 215-217: storeSubscriptionForMeeting currently treats
addToSubscriptionIndex failures as non-fatal which leaves KV state inconsistent;
change storeSubscriptionForMeeting so that if addToSubscriptionIndex(userID,
meetingID) returns an error it does not return success — either roll back the
prior subscription write (call the inverse/remove function such as
removeSubscriptionForMeeting or delete the created KV entry) and return the
error, or return the error immediately so the overall operation fails; update
any callers to handle the error accordingly so subscription list (used by
command handlers) stays in sync.
In `@server/webhook.go`:
- Around line 378-381: The downloadZoomFile function currently uses
io.LimitReader which silently truncates large responses; change the read to read
up to maxDownloadSize+1 bytes (use io.LimitReader(response.Body,
maxDownloadSize+1) or equivalent) into a buffer, then check if len(data) >
maxDownloadSize and return an explicit "payload too large" error before
proceeding; reference the symbols downloadZoomFile, response.Body, and
maxDownloadSize when making this change so oversized downloads are rejected
instead of silently truncated.
- Around line 498-507: The code inside the loop overwrites newPost.Message for
each MP4 recording so only the last link remains; modify the handling in the
loop that processes recording.PlayURL (where you call p.isZoomDownloadURL and
build msg using webhook.Payload.Object.Password and
p.getConfiguration().EnablePostingRecordingPassword) to append each constructed
msg to newPost.Message instead of assigning it (e.g., join with a newline or
bullet separator), preserving earlier MP4 links and their optional password
lines for the group.
In `@server/zoom/webhook.go`:
- Around line 73-75: The RecordingWebhook struct has wrong tags: change the Type
and Payload struct tags from schema:"..." to json:"..." so JSON unmarshaling
populates them; update the RecordingWebhook definition (fields Type and Payload)
to use json:"type" and json:"content" respectively so handleTranscriptCompleted
and handleRecordingCompleted receive the data correctly.
---
Nitpick comments:
In `@server/plugin_test.go`:
- Around line 173-174: The KV mocks for KVGet and KVSetWithExpiry are too
permissive—replace the broad mock.MatchedBy(func(key string) bool { return
strings.HasPrefix(key, meetingChannelKey) }).Maybe() with tighter expectations:
assert the exact expected key(s) or use mock.MatchedBy predicates that validate
the full constructed key (including room/meeting IDs) and set explicit call
counts (e.g., Once() or Times(n)) instead of Maybe(); update the KVSetWithExpiry
matcher to check the byte payload/type (instead of mock.AnythingOfType) and the
exact TTL constant adHocMeetingChannelTTL so tests fail on unexpected KV
interactions and detect regressions involving KVGet/KVSetWithExpiry.
- Around line 68-71: The tests use trivial UUIDs ("234") which won't catch
normalization/lookup bugs; update the test data around endedPayload,
validStoppedWebhookRequest and validStartedWebhookRequest to include at least
one realistic Zoom-style UUID containing special characters (e.g., characters
like /, +, =, or underscores) and add assertions or an additional test request
that uses that UUID shape so webhook-to-KV mapping logic is exercised; locate
and modify the variables endedPayload, validStoppedWebhookRequest and
validStartedWebhookRequest in the test to include the new payload string(s).
In `@server/webhook_test.go`:
- Around line 381-459: The test TestWebhookHandleTranscriptCompleted is missing
explicit HTTP status assertions; after calling p.ServeHTTP(&plugin.Context{}, w,
request) and reading the response body, assert the expected HTTP status (e.g.,
200 OK or the specific code your handler should return) using
w.Result().StatusCode (and optionally check w.Result().Status) so failures
surface in test output; apply the same pattern to the other webhook tests
referenced (lines 461-531) to harden them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 53393f3d-a197-4730-a49f-74c54a9a2f7e
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
plugin.jsonserver/command.goserver/configuration.goserver/http.goserver/plugin.goserver/plugin_test.goserver/store.goserver/telemetry.goserver/webhook.goserver/webhook_test.goserver/zoom/meeting.goserver/zoom/webhook.gowebapp/.eslintrc.jsonwebapp/package.jsonwebapp/src/components/ai_icon.tsxwebapp/src/components/ai_summary_button.tsxwebapp/src/components/post_type_chat.tsxwebapp/src/components/post_type_transcription.tsxwebapp/src/components/post_type_zoom/post_type_zoom.jsxwebapp/src/components/svg.tsxwebapp/src/index.jswebapp/tsconfig.jsonwebapp/webpack.config.js
|
@coderabbitai full review please |
|
The PR description images show:
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/webhook.go (1)
124-127:⚠️ Potential issue | 🟠 MajorGuard against empty UUID before storing the meeting→post mapping.
At Line 126,
storeMeetingPostIDis called even whenwebhook.Payload.Object.UUIDmay be empty, which can recreate a colliding key and misassociate later webhooks.Suggested fix
if !entry.IsSubscription { if existingPostID, err := p.findMeetingPostByMeetingID(meetingID); err == nil { - if appErr := p.storeMeetingPostID(webhook.Payload.Object.UUID, existingPostID); appErr != nil { - p.API.LogWarn("failed to store UUID mapping for existing post", - "error", appErr.Error(), - ) - } + if webhook.Payload.Object.UUID != "" { + if appErr := p.storeMeetingPostID(webhook.Payload.Object.UUID, existingPostID); appErr != nil { + p.API.LogWarn("failed to store UUID mapping for existing post", + "error", appErr.Error(), + ) + } + } else { + p.API.LogWarn("handleMeetingStarted: empty UUID, skipping post mapping", + "meeting_id", meetingID, + "post_id", existingPostID, + ) + } w.WriteHeader(http.StatusOK) return } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/webhook.go` around lines 124 - 127, The code calls p.storeMeetingPostID with webhook.Payload.Object.UUID even when that UUID may be empty; update the handler (the branch using entry.IsSubscription, p.findMeetingPostByMeetingID and p.storeMeetingPostID) to first verify webhook.Payload.Object.UUID is non-empty (e.g., != "" or len(...) > 0) before invoking p.storeMeetingPostID, and if it is empty skip the store and log a warning via p.API.LogWarn (including context like meetingID and existingPostID) so you avoid creating a colliding/invalid mapping.
🧹 Nitpick comments (3)
webapp/src/components/ai_icon.tsx (1)
9-14: Hide decorative SVG from assistive technologies.Since this icon is paired with visible button text, mark it decorative to avoid redundant announcements by screen readers.
Suggested fix
<Svg width='15' height='15' viewBox='0 0 19 19' fill='none' + aria-hidden='true' + focusable='false' >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/ai_icon.tsx` around lines 9 - 14, The SVG icon is not marked decorative, causing screen readers to announce it redundantly; update the Svg element in ai_icon.tsx (the <Svg ...> used in the ai icon component) to be ignored by assistive tech by adding accessibility attributes such as aria-hidden="true" and focusable="false" (do not add a role) so the icon is treated as decorative when paired with visible button text.server/command.go (1)
289-292: Meeting ID parsing may accept unintended formats.Using
strings.Join(params[1:], "")beforestrconv.Atoiconcatenates all remaining arguments without spaces. This means/zoom subscription add 123 456would parse as meeting ID123456instead of rejecting the command. Consider validating that exactly one argument is provided.♻️ Suggested improvement
- meetingID, err := strconv.Atoi(strings.Join(params[1:], "")) - if err != nil { - return "Invalid meeting ID. Please provide a numeric meeting ID.", nil - } + if len(params) != 2 { + return fmt.Sprintf("Please specify exactly one meeting ID. Usage: `/zoom subscription %s [meetingID]`", action), nil + } + meetingID, err := strconv.Atoi(params[1]) + if err != nil { + return "Invalid meeting ID. Please provide a numeric meeting ID.", nil + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/command.go` around lines 289 - 292, The meeting ID parsing currently concatenates all remaining params with strings.Join(params[1:], "") and passes that to strconv.Atoi, which allows multi-argument inputs like "123 456" to become "123456"; change the logic to require exactly one argument after the command (check len(params) == 2), take the single token params[1] (trim whitespace), then call strconv.Atoi on that token and return the current error message if conversion fails or if the wrong number of arguments is provided; update the handling around meetingID, strconv.Atoi, and params usage accordingly.server/store.go (1)
357-368: Minor inefficiency:removeFromSubscriptionIndexalways writes back the index.Unlike
addToSubscriptionIndexwhich returnsnilwhen the item already exists (avoiding unnecessary writes),removeFromSubscriptionIndexalways returns the filtered index even ifmeetingIDwasn't present, causing an unnecessary KV write operation.♻️ Suggested optimization
func (p *Plugin) removeFromSubscriptionIndex(userID string, meetingID int) error { return p.updateSubscriptionIndex(userID, func(idx *subscriptionIndex) *subscriptionIndex { + found := false filtered := make([]int, 0, len(idx.MeetingIDs)) for _, id := range idx.MeetingIDs { if id != meetingID { filtered = append(filtered, id) + } else { + found = true } } + if !found { + return nil + } idx.MeetingIDs = filtered return idx }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/store.go` around lines 357 - 368, The removeFromSubscriptionIndex function always returns a modified subscriptionIndex causing a KV write even when meetingID wasn't present; update the closure passed to updateSubscriptionIndex (used by removeFromSubscriptionIndex) to detect whether any MeetingIDs were actually removed and return nil when no change occurred, otherwise return the updated *subscriptionIndex; reference subscriptionIndex.MeetingIDs, removeFromSubscriptionIndex, and updateSubscriptionIndex to implement the existence check (e.g., compare lengths or track a removed flag) so unnecessary writes are avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/store.go`:
- Around line 313-318: The code currently swallows json.Unmarshal failures into
an empty subscriptionIndex and proceeds to call mutate(&idx), risking
overwriting valid data; change the error handling in the block around
json.Unmarshal(oldRaw, &idx) so that on unmarshal error you do not continue the
atomic update—log the error with p.API.LogWarn/p.API.LogError including
err.Error() and return or propagate an error (abort the update) instead of
falling through to mutate(&idx), ensuring that subscriptionIndex is only mutated
when unmarshalling succeeds.
In `@webapp/src/components/ai_summary_button.tsx`:
- Line 86: The SummaryButton rendered in the SummaryButton onClick={handleClick}
usage lacks an explicit type and therefore defaults to "submit"; update the
SummaryButton element to include type="button" so clicks do not trigger form
submissions (e.g., change the JSX where SummaryButton is rendered to pass
type="button" alongside onClick={handleClick}).
---
Duplicate comments:
In `@server/webhook.go`:
- Around line 124-127: The code calls p.storeMeetingPostID with
webhook.Payload.Object.UUID even when that UUID may be empty; update the handler
(the branch using entry.IsSubscription, p.findMeetingPostByMeetingID and
p.storeMeetingPostID) to first verify webhook.Payload.Object.UUID is non-empty
(e.g., != "" or len(...) > 0) before invoking p.storeMeetingPostID, and if it is
empty skip the store and log a warning via p.API.LogWarn (including context like
meetingID and existingPostID) so you avoid creating a colliding/invalid mapping.
---
Nitpick comments:
In `@server/command.go`:
- Around line 289-292: The meeting ID parsing currently concatenates all
remaining params with strings.Join(params[1:], "") and passes that to
strconv.Atoi, which allows multi-argument inputs like "123 456" to become
"123456"; change the logic to require exactly one argument after the command
(check len(params) == 2), take the single token params[1] (trim whitespace),
then call strconv.Atoi on that token and return the current error message if
conversion fails or if the wrong number of arguments is provided; update the
handling around meetingID, strconv.Atoi, and params usage accordingly.
In `@server/store.go`:
- Around line 357-368: The removeFromSubscriptionIndex function always returns a
modified subscriptionIndex causing a KV write even when meetingID wasn't
present; update the closure passed to updateSubscriptionIndex (used by
removeFromSubscriptionIndex) to detect whether any MeetingIDs were actually
removed and return nil when no change occurred, otherwise return the updated
*subscriptionIndex; reference subscriptionIndex.MeetingIDs,
removeFromSubscriptionIndex, and updateSubscriptionIndex to implement the
existence check (e.g., compare lengths or track a removed flag) so unnecessary
writes are avoided.
In `@webapp/src/components/ai_icon.tsx`:
- Around line 9-14: The SVG icon is not marked decorative, causing screen
readers to announce it redundantly; update the Svg element in ai_icon.tsx (the
<Svg ...> used in the ai icon component) to be ignored by assistive tech by
adding accessibility attributes such as aria-hidden="true" and focusable="false"
(do not add a role) so the icon is treated as decorative when paired with
visible button text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b6777210-53b8-4cc6-97ff-6ac90a26d0db
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
plugin.jsonserver/command.goserver/configuration.goserver/http.goserver/plugin.goserver/plugin_test.goserver/store.goserver/telemetry.goserver/webhook.goserver/webhook_test.goserver/zoom/meeting.goserver/zoom/webhook.gowebapp/.eslintrc.jsonwebapp/package.jsonwebapp/src/components/ai_icon.tsxwebapp/src/components/ai_summary_button.tsxwebapp/src/components/post_type_chat.tsxwebapp/src/components/post_type_transcription.tsxwebapp/src/components/post_type_zoom/post_type_zoom.jsxwebapp/src/components/svg.tsxwebapp/src/index.jswebapp/tsconfig.jsonwebapp/webpack.config.js
matthewbirtch
left a comment
There was a problem hiding this comment.
This is great @avasconcelos114. This will be a nice improvement to zoom meetings in mattermost
There was a problem hiding this comment.
@avasconcelos114 Great work! I only have those two request changes :)
| return "You do not have permission to subscribe to this channel", nil | ||
| } | ||
|
|
||
| meeting, err := p.getMeeting(user, meetingID) |
There was a problem hiding this comment.
Since the plugin can operate in account-level OAuth mode (where I think getActiveClient uses a superuser token), getMeeting() will succeed for any meeting in the Zoom account - not just meetings owned by the user running the command. I think this means any Mattermost user could subscribe a channel to someone else's meeting by knowing its numeric ID.
The concern goes beyond "meeting started" notifications: when recordings and transcripts complete, handleRecordingCompleted and handleTranscriptCompleted post them as replies to the original meeting post - so the subscribing user would also receive the video recording (with password if enabled), chat history, and transcript of a meeting they weren't part of.
Would it make sense to check that meeting.HostID matches the Zoom user linked to the Mattermost user (or that the user is a system admin) before allowing the subscription?
There was a problem hiding this comment.
That's a great point, I think it will be good to start out conservative with allowing only users who are the hosts and sys admins to create subscriptions, i'll make the changes needed
| return | ||
| } | ||
|
|
||
| if postMeetingErr := p.postMeeting(botUser, meetingID, webhook.Payload.Object.UUID, channelID, "", webhook.Payload.Object.Topic); postMeetingErr != nil { |
There was a problem hiding this comment.
Small concern: when the webhook path calls postMeeting(botUser, ...), the permission check in postMeeting is skipped because creator.Id == botUserID. The channel permission was validated at subscription time, but if the user who created the subscription later loses access to the channel, the bot will keep posting there indefinitely since subscriptions have no TTL.
A lightweight fix could be to re-check that entry.CreatedBy still has PermissionCreatePost on the channel before posting:
if entry.IsSubscription && entry.CreatedBy != "" {
if !p.API.HasPermissionToChannel(entry.CreatedBy, entry.ChannelID, model.PermissionCreatePost) {
p.API.LogWarn("subscription creator lost channel access, skipping post")
w.WriteHeader(http.StatusOK)
return
}
}
|
Thank you both on these reviews too! Let me know if there is anything else that would be good to address :) |
edgarbellot
left a comment
There was a problem hiding this comment.
Great job Andre! All good - thank you again! :)
nevyangelova
left a comment
There was a problem hiding this comment.
Thanks @avasconcelos114 awesome work :))
ogi-m
left a comment
There was a problem hiding this comment.
Thanks Andre, LGTM!
- Ad-hoc meeting (/zoom start): verified that after the meeting ends, both the transcript and recording links are posted as thread replies to the original meeting post
- AI summarization: confirmed that (when the agents plugin is available) the "Create meeting summary" button appears on transcript post.
- Subscription commands: tested /zoom subscription add, /zoom subscription remove, and /zoom subscription list — all work as expected
- Subscribed recurring meeting: verified that when a subscribed meeting starts, a meeting post is automatically created in the subscribed channel by the bot
- Subscribed meeting post-meeting content: confirmed that transcript and recording links are posted as thread replies to the subscription-created meeting post after the meeting ends
Summary
This PR is a continuation of #377, synced with the latest changes from
masterand then a few additional things reworked / improved as part of the process/zoom subscriptioncommands under (add,remove, andlistoptions)/zoom startcommandAs a part of these changes I also created a new PR for the docs that continues what was started in mattermost/docs#7276
Documentation PR Link: mattermost/docs#8763
Screenshots
Ticket Link
Fixes https://mattermost.atlassian.net/browse/MM-67409
QA Notes
In order to test this, a user-managed Zoom application has to be created with all the needed scopes (should also work with an Admin-managed application and its own related scopes available but I ran into permission issues with Zoom):
meeting:read:meetingmeeting:write:meetinguser:read:usercloud_recording:read:recordingarchiving:read:list_archived_filesPlease note that cloud recordings require a paid Zoom account in order to test, so feel free to reach out to me when testing so I can use a Zoom application I've already created to connect it to a MM test instance.
I'll list down a list of tests I performed as a part of working on this, but please let me know if you'd like more detailed instructions
/zoom start)Change Impact: 🔴 High
Reasoning: The changes span core server and webapp layers—webhooks, storage keying/indexing, HTTP handling, download/upload clients, public plugin method signatures, and UI post types—introducing new end-to-end behaviors (UUID-based meeting-post mapping, user-scoped subscriptions, recording/transcript download & upload, and AI summarization) that significantly increase cross-cutting complexity.
Regression Risk: High — modifications to persistence keys and indices, webhook routing/signature handling, file download/upload flows, and altered storage/API signatures create substantial risk of subtle breakages in meeting-post resolution, recording/transcript delivery, subscription ownership behavior, and backwards compatibility with existing stored data.
QA Recommendation: Perform extensive manual and automated testing before rollout: exercise PMI/ad-hoc/recurring meetings and replays of webhook sequences (meeting.started/ended, recording.completed, recording.transcript_completed); validate subscription add/list/remove across user roles; verify recording/transcript download, upload, and posted password behavior (note cloud recordings require paid Zoom account); and confirm AI summary button behavior with the agents plugin enabled. Do not skip manual QA for these critical flows.
Generated by CodeRabbitAI