Skip to content

MM-68540: Stop pushing sidebar data over cluster WS; refetch via REST #1002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nang2049 merged 1 commit into from
May 6, 2026
+4 −57
Merged

MM-68540: Stop pushing sidebar data over cluster WS; refetch via REST #1002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 2 additions & 50 deletions server/plugin/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package plugin

import (
"context"
"encoding/json"
"fmt"
"net/http"
"net/url"
Expand All @@ -25,7 +24,6 @@ import (
"github.com/mattermost/mattermost/server/public/plugin"
"github.com/mattermost/mattermost/server/public/pluginapi"
"github.com/mattermost/mattermost/server/public/pluginapi/cluster"
"github.com/mattermost/mattermost/server/public/pluginapi/experimental/bot/logger"
"github.com/mattermost/mattermost/server/public/pluginapi/experimental/bot/poster"

"github.com/mattermost/mattermost-plugin-github/server/plugin/graphql"
Expand Down Expand Up @@ -1305,66 +1303,20 @@ func (p *Plugin) isOrganizationLocked() bool {
}

func (p *Plugin) sendRefreshEvent(userID string) {
eventLogger := logger.New(p.API).With(logger.LogContext{
"userid": userID,
})

ctx, cancel := context.WithTimeout(context.Background(), requestTimeout)

context := &Context{
Ctx: ctx,
UserID: userID,
Log: eventLogger,
}

defer cancel()

info, apiErr := p.getGitHubUserInfo(context.UserID)
if apiErr != nil {
if _, apiErr := p.getGitHubUserInfo(userID); apiErr != nil {
if apiErr.ID != apiErrorIDNotConnected {
p.client.Log.Debug("Failed to get github user info", "error", apiErr.Error())
}
return
}

userContext := &UserContext{
Context: *context,
GHInfo: info,
}

sidebarContent, err := p.getSidebarData(userContext)
if err != nil {
p.client.Log.Warn("Failed to get the sidebar data", "error", err.Error())
return
}

contentMap, err := sidebarContent.toMap()
if err != nil {
p.client.Log.Warn("Failed to convert sidebar content to map", "error", err.Error())
return
}

p.client.Frontend.PublishWebSocketEvent(
wsEventRefresh,
contentMap,
map[string]any{"user_id": userID},
Copy link
Copy Markdown
Member

@lieut-data lieut-data May 5, 2026

Choose a reason for hiding this comment

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

Do we even need to emit this at all, given the receiving client knows to fetch their own updated data?

Copy link
Copy Markdown
Contributor Author

@nang2049 nang2049 May 6, 2026

Choose a reason for hiding this comment

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

Thank you for your question, its still needed becase it's the only signal the client has that a webhook arrived. Without it the sidebar wouldn't update until reconnect or a manual refresh.

Copy link
Copy Markdown
Member

@lieut-data lieut-data May 6, 2026

Choose a reason for hiding this comment

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

Sorry, the websocket is expected, but telling the user their own user_id is not.

Copy link
Copy Markdown
Contributor Author

@nang2049 nang2049 May 6, 2026

Choose a reason for hiding this comment

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

You're right followup PR to drop it #1004

&model.WebsocketBroadcast{UserId: userID},
)
}

func (s *SidebarContent) toMap() (map[string]any, error) {
var m map[string]any
bytes, err := json.Marshal(&s)
if err != nil {
return nil, err
}

if err = json.Unmarshal(bytes, &m); err != nil {
return nil, err
}

return m, nil
}

// getUsername returns the GitHub username for a given Mattermost user,
// if the user is connected to GitHub via this plugin.
// Otherwise it return the Mattermost username. It will be escaped via backticks.
Expand Down
9 changes: 2 additions & 7 deletions webapp/src/websocket/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,9 @@ export function handleReconnect(store, reminder = false) {
}

export function handleRefresh(store) {
return (msg) => {
return () => {
if (store.getState()[`plugins-${manifest.id}`].connected) {
const {data} = msg;

store.dispatch({
type: ActionTypes.RECEIVED_SIDEBAR_CONTENT,
data,
});
getSidebarContent()(store.dispatch, store.getState);
}
};
}
Expand Down
Loading