Improve client call logs by bgardner8008 · Pull Request #1126 · mattermost/mattermost-plugin-calls

bgardner8008 · 2026-02-05T20:49:00Z

Summary

This PR makes a number of improvements to the "/call logs" implementation, including retaining logs for both failed and successful calls, accumulating consecutive call logs while truncating to limit total log length, uploading as a file if logs are too long to post, including call stats with logs, including log messages that occur during initialization before call starts, and adding unit tests. Please see Jira ticket for more details.

Ticket Link

https://mattermost.atlassian.net/browse/MM-67452

…ich is length limited, upload via file if too long to post

codecov-commenter · 2026-02-05T20:57:30Z

Codecov Report

❌ Patch coverage is 15.82734% with 117 lines in your changes missing coverage. Please review.
✅ Project coverage is 23.85%. Comparing base (53dbaad) to head (67b4391).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
server/api.go	0.00%	72 Missing ⚠️
server/slash_command.go	0.00%	17 Missing ⚠️
webapp/src/slash_commands.tsx	0.00%	13 Missing ⚠️
webapp/src/websocket_handlers.ts	0.00%	8 Missing ⚠️
webapp/src/index.tsx	0.00%	4 Missing ⚠️
webapp/src/client.ts	0.00%	2 Missing ⚠️
webapp/src/log.ts	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1126      +/-   ##
==========================================
- Coverage   23.99%   23.85%   -0.14%     
==========================================
  Files         237      237              
  Lines       13532    13655     +123     
  Branches     1611     1614       +3     
==========================================
+ Hits         3247     3258      +11     
- Misses       9978    10094     +116     
+ Partials      307      303       -4

Files with missing lines	Coverage Δ
server/api_router.go	`56.31% <100.00%> (+0.86%)`	⬆️
webapp/src/constants.ts	`100.00% <100.00%> (ø)`
webapp/src/types/types.ts	`100.00% <ø> (ø)`
webapp/src/log.ts	`93.10% <94.73%> (+56.26%)`	⬆️
webapp/src/client.ts	`13.92% <0.00%> (-0.05%)`	⬇️
webapp/src/index.tsx	`0.00% <0.00%> (ø)`
webapp/src/websocket_handlers.ts	`0.00% <0.00%> (ø)`
webapp/src/slash_commands.tsx	`0.00% <0.00%> (ø)`
server/slash_command.go	`0.00% <0.00%> (ø)`
server/api.go	`0.74% <0.00%> (-0.17%)`	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lieut-data

This looks great, @bgardner8008! A few thoughts below to consider before merging.

lieut-data · 2026-02-06T15:17:23Z

webapp/src/types/types.ts


 export type CallsClientStats = {
    initTime: number;
-    callID: string;


To confirm, did we used to have a callID that was unique per call, or are we just changing this to match the channelID it ended up always being? Should we preserve both? (I dream of being able to filter logs by call_id=... to get details on exactly this call, and separately on channel_id=... to get all calls in a channel.)

I assumed this meant the actual callID, which we now log in each RTCD entry, but when debugging recently I discovered that it is in fact the channelID.

public async getStats(): Promise<CallsClientStats | null> { // ... return { initTime: this.initTime, channelID: this.channelID, tracksInfo, rtcStats: stats ? parseRTCStats(stats) : null, };

So in fact debugging from stats requires finding a particular message in RTCD that gives the mapping from channelID to callID. But... does callID exist in the client? Yes it does. I'm looking to include it directly in the stats, making sure not to break anything.

Yep... the whole "callID == channelID, except in other places where it's actually a unique per-call ID" thing... It's definitely something we've needed to go through and fix, it was just baked in to so much that we never got around to it.

lieut-data · 2026-02-06T15:19:43Z

webapp/src/slash_commands.tsx

+        if (allLogs.length < MAX_INLINE_LOG_POST_SIZE) {
+            // Small enough - post inline (existing behavior)
+            return {message: `/call logs ${btoa(allLogs)}`, args};
+        }


Should we just always post an attachment? I can always click to view it, and then it's a uniform experience, easy to share "one way" even if it happens to be small.

Thanks for this. I actually like the file upload better and I will implement that.

lieut-data · 2026-02-06T15:20:57Z

webapp/src/slash_commands.tsx

+                channel_id: args.channel_id,
+                message: `📋 Call logs (${sizeKB} KB)`,
+                file_ids: [fileId],
+            } as any);


This has one potential downside that my logs are noq public to the channel. Could they be sensitive in any way? Should we consider posting this to the DM with the @calls bot instead, guaranteeing privacy? (We could then post an ephemeral reply to the channel for the user to find it.)

So... when posting as text it is not available to others? I see... so yes this is a big change. I'll take a look at your suggestions.

Here's a way it could work. The "/call logs" command would upload the file via post to calls api endpoint which would store it in KV store with a TTL expiration and return an ephemeral message with link to file with anonymous URL. User could then download themselves via link or copy the link and message it to another user, say calls team, for download and analysis. The file is deleted after TTL, 7 days say.

I'd steer away from the KV store idea, and just use the DM with the @calls bot as an equivalent "stash". The idea is that when you do /call stats, the file will be attached to a post there, and you'll get an ephemeral message where you ran the slash command (as well as a notification) helping you find it.

This solves the security problem, avoids any clean up, and generally just fits well into our current data model of using the DM with a bot as a kind of "side channel".

I can understand if you're opposed to saving file data in the KV DB, even if it has TTL. If I understand correctly, DMing the bot with a file will store the file in s3 and post ref to file in DM channel. The file will live forever, depending on data retention policies. And the post is completely inaccessible to other users, i.e., the user can't make a link to the post and send that. If the user wants to send the file to say the calls team they would have to download the file and repost the file in a new message. I just want to check you weren't thinking of wiring up some automation logic to the bot channel to forward logs. While that might be possible I don't want to get automatic notifications of logs and this just seems way too complicated.

I'll implement the bot stash method.

And the post is completely inaccessible to other users, i.e., the user can't make a link to the post and send that. If the user wants to send the file to say the calls team they would have to download the file and repost the file in a new message.

Ah, this is a good point, @bgardner8008 -- I hadn't thought about the cost of re-sharing now.

Actually, how sensitive really is the data in question? Is there a universe where we just post it to the channel where you run it, and run with that to keep things simple? (No KV store, no "bot stash" required.)

Best to keep private given concerns from security. I've implemented the bot upload and it seems to work well. I also tried direct DL from blob which was best solution but only works on webapp as electron blocks the blob download.

OK, the latest commit uploads to @Calls bot DM channel, and the calls plugin gets a fileID for the upload and makes a link for user to click on to download. It so happens these links are shareable, and secure due to use of anonymous fileID. So this latest implementation works nicely: log file is uploaded and there's an ephemeral post containing link for user to download if they wish, or they can copy the link and message it to someone else.

A few notes: 1) There is no post made to the @Calls bot DM channel, so if the users navigates to that channel there is nothing to see. 2) When DLing log from webapp, the browser will also open the file to view in a texteditor, while the desktop app just DLs and queries user for location.

Oh, even better! Nice :)

webapp/src/log.ts

lieut-data · 2026-02-06T20:33:44Z

webapp/src/index.tsx

            // Desktop handler
            const payload = {
-                callID: channelID,
+                callID: callID || channelID, // use actual callID if available, fallback to channelID for compatibility


How is this callID actually used in the payload? Is this actually a case we want to change? At first glance, I could imagine the "desktop handler" on the other side actually does assume this as a channelId, but I admit I haven't looked at that code.

I see now why channelID was used for stats. The callID is stored in Redux state when client created, but... at this point it hasn't been set, so it's undefined. So I'm guessing channel ID was used instead as a unique ID in the code, because it is known at the very start. The callID is known later when the websocket message start_call comes in. But it looks like channelID is used throughout the code for what was supposed to be callID, makes for lots of confusion.

That line 685 above is indeed a mess and must go. I think I will revert all changes regarding callID v channelID and simply make sure the callID is logged when known, and leave the channelID usage as is, otherwise I'll just break something.

CallID is sent in call_start message when a call is started on a channel. It is also sent in call_state message which are state updates that can be requested by client and are also sent by server to a user client after that user joins. But callID is not sent on user_joined message, which are broadcast to all users in channel. So if a user navigates to a channel and joins a call, the client doesn't know the callID until sometime later when a call_state message comes in.

lieut-data · 2026-02-06T20:40:28Z

webapp/src/log.ts

+    // Clear accumulated background noise once per day if not in a call
+    setInterval(() => {
+        if (!window.callsClient) {
+            clientLogs = '';


I worry about the race condition where this function cleans up right after a call but before I can grab any issues. NOT worth over engineering given the upcoming pivot, but maybe we can at least emit a console.log statement when we clear a non-empty log just to it's traceable.

If the timer fires exactly when the call has started to initialize but before window.callsClient is created, then we could clear some initialization log messages, but these were never making it to log before. Previously log messages were stored in memory only when window.callsClient existed. It's not worth the time spent discussing honestly, I'll just delete the timer.

This reverts commit 4925f5a.

… post with DL link

lieut-data · 2026-02-09T18:49:57Z

@bgardner8008, whenever you're ready for me to re-review, feel free to re-request (otherwise I'll assume you're still iterating):

lieut-data

Thanks, @bgardner8008 -- an important blocker below:

lieut-data · 2026-02-09T19:19:36Z

server/api.go

+	// Get the file content from Mattermost
+	fileData, appErr := p.API.GetFile(fileID)
+	if appErr != nil {
+		http.Error(w, fmt.Sprintf("Failed to get file: %s", appErr.Error()), http.StatusNotFound)
+		return
+	}
+
+	// Get file info for the filename
+	fileInfo, appErr := p.API.GetFileInfo(fileID)
+	if appErr != nil {
+		http.Error(w, fmt.Sprintf("Failed to get file info: %s", appErr.Error()), http.StatusInternalServerError)
+		return
+	}
+


Unfortunately, this effectively allows anyone to download any file given the file_id. Instead of exposing a download endpoint, can we leverage the "public link" functionality for files and send that directly back to the client?

Hmm, I see, that is a problem. I'll have to study the "public link" idea. Another idea would be to have the download endpoint get the file ID through KV entries (or some table of log files) that are created only for log files using a different ID, so that only log files could be accessed through that API endpoint.

I implemented the public link idea by attaching the file to a post in the bot DM channel, so now they are visible, and creating public link. That works great, and gives shareable links, but the whole mechanism requires public links to be enabled in settings. So I reverted to the log download endpoint, but now the logs are served only if 1) user is logged in, 2) calls bot exists, 3) file belongs to DM channel between user and calls bot. So all the user can do is download the log that is posted in the DM channel to the calls bot (sharing link not supported). I also thought about just linking to the calls bot channel but that is poor user experience to have to navigate there first to DL file.

I want to address chris' point about object logging and then I request a re-review.

lieut-data

Thanks, @bgardner8008 -- an important blocker above.

cpoile

Thanks Bill and Jesse -- thorough review, and things look good! (assuming Jesse's concerns are addressed)

cpoile · 2026-02-09T16:11:46Z

webapp/src/types/types.ts


 export type CallsClientStats = {
    initTime: number;
-    callID: string;


Yep... the whole "callID == channelID, except in other places where it's actually a unique per-call ID" thing... It's definitely something we've needed to go through and fix, it was just baked in to so much that we never got around to it.

cpoile · 2026-02-09T20:42:37Z

webapp/src/log.test.ts

+
+            const accumulated = getClientLogs();
+            expect(accumulated).toContain('object:');
+            expect(accumulated).toContain('[object Object]');


just curious if this is the best we can do for logging something useful for objects? totally fine if it is, but thought I'd ask.

Thanks for pointing that out, those generic "object" logs are not helpful.

…nd calls bot

… /call logs base64-data

bgardner8008 added 2 commits February 5, 2026 11:37

logging improvements, record all call logs and stats in one buffer wh…

0408925

…ich is length limited, upload via file if too long to post

add tests for log accumulation and truncation

b9faf19

fix linter issues

a7b56af

bgardner8008 requested review from cpoile and lieut-data February 6, 2026 15:15

lieut-data requested changes Feb 6, 2026

View reviewed changes

add callID to stats, via CallsClientJoinData

4925f5a

lieut-data reviewed Feb 6, 2026

View reviewed changes

bgardner8008 added 5 commits February 6, 2026 18:11

Revert "add callID to stats, via CallsClientJoinData"

b9ca2bb

This reverts commit 4925f5a.

log IDs on call_start and call_join

c4a4d86

implement log file upload to @Calls bot DM channel and make ephemeral…

3a19639

… post with DL link

remove periodic cleanup of in-memory log

de27793

fix linter issue

a4cffec

bgardner8008 requested a review from lieut-data February 9, 2026 18:51

lieut-data requested changes Feb 9, 2026

View reviewed changes

cpoile approved these changes Feb 9, 2026

View reviewed changes

bgardner8008 added 2 commits February 23, 2026 17:02

restrict log downloads to files that are in DM channel between user a…

a01838a

…nd calls bot

add validation to handleLogsCommand to prevent malicious injection to…

67b4391

… /call logs base64-data

Conversation

bgardner8008 commented Feb 5, 2026

Summary

Ticket Link

Uh oh!

codecov-commenter commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lieut-data left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lieut-data Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lieut-data commented Feb 9, 2026

Uh oh!

lieut-data left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lieut-data left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpoile left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov-commenter commented Feb 5, 2026 •

edited

Loading

lieut-data Feb 6, 2026 •

edited

Loading

lieut-data left a comment •

edited

Loading

cpoile left a comment •

edited

Loading