Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
119 changes: 119 additions & 0 deletions pkg/connector/avatars.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
package connector

import (
"context"
"fmt"
"net/http"
"time"

"github.com/rs/zerolog"
"maunium.net/go/mautrix/bridgev2"
"maunium.net/go/mautrix/bridgev2/networkid"
)

const (
redditorBatchSize = 50
avatarRetryDelay = 5 * time.Minute
)

var avatarHTTPClient = &http.Client{Timeout: 30 * time.Second}

// Reddit's chat Matrix server returns no avatars, so they come from the web
// session. A missing icon is cached as empty so it isn't requested on every
// resync, and failed lookups back off rather than repeat on every sync batch.
func (r *RedditClient) redditorAvatars(ctx context.Context, userIDs []networkid.UserID) map[networkid.UserID]*bridgev2.Avatar {
if r.session == nil {
return nil
}
if missing := r.claimUncachedRedditors(userIDs); len(missing) > 0 {
r.fetchRedditorIcons(ctx, missing)
}
r.avatarMu.Lock()
defer r.avatarMu.Unlock()
Comment thread
indent[bot] marked this conversation as resolved.
Resolved
avatars := make(map[networkid.UserID]*bridgev2.Avatar, len(userIDs))
for _, userID := range userIDs {
if url := r.avatarURLs[userID]; url != "" {
avatars[userID] = &bridgev2.Avatar{
ID: networkid.AvatarID(url),
Get: func(ctx context.Context) ([]byte, error) { return downloadRedditorAvatar(ctx, url) },
}
}
}
return avatars
}

// The lookup itself runs without avatarMu, which the sync path takes. Claimed
// IDs are skipped by concurrent callers until it finishes.
func (r *RedditClient) claimUncachedRedditors(userIDs []networkid.UserID) []string {
r.avatarMu.Lock()
defer r.avatarMu.Unlock()
if time.Now().Before(r.avatarRetryAt) {
return nil
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪️ Nit · Only the first 50 uncached members get avatars per chat sync

claimUncachedRedditors stops after redditorBatchSize (50) IDs and redditorAvatars makes one fetch per call. In a group with more than 50 members, one sync fetches avatars for 50 members, chosen by Go's random map order. The rest only fill in 50 at a time as later room updates trigger resyncs, so quiet large groups can stay partly avatar-less for a long time. The previous commit chunked through every missing ID.

Claim all uncached IDs and fetch them in chunks outside the lock, releasing the claims for unfetched batches on error. Or loop until nothing is left to claim.

Found by Indent Review Agent

if r.avatarURLs == nil {
r.avatarURLs = make(map[networkid.UserID]string)
r.avatarFetching = make(map[networkid.UserID]bool)
}
var missing []string
for _, userID := range userIDs {
if len(missing) == redditorBatchSize {
break
}
if _, cached := r.avatarURLs[userID]; !cached && !r.avatarFetching[userID] && isRedditUserID(userID) {
r.avatarFetching[userID] = true
missing = append(missing, string(userID))
}
}
return missing
}

func (r *RedditClient) fetchRedditorIcons(ctx context.Context, redditorIDs []string) {
icons, err := r.session.GetRedditorIcons(ctx, redditorIDs)
if err != nil {
zerolog.Ctx(ctx).Warn().Err(err).Msg("Failed to fetch Reddit avatars")
}
r.avatarMu.Lock()
defer r.avatarMu.Unlock()
if err != nil {
r.avatarRetryAt = time.Now().Add(avatarRetryDelay)
}
for _, redditorID := range redditorIDs {
delete(r.avatarFetching, networkid.UserID(redditorID))
if err == nil {
r.avatarURLs[networkid.UserID(redditorID)] = icons[redditorID]
}
}
}

func (r *RedditClient) addMemberAvatars(ctx context.Context, info *bridgev2.ChatInfo) *bridgev2.ChatInfo {
if info.Members == nil {
return info
}
userIDs := make([]networkid.UserID, 0, len(info.Members.MemberMap))
for _, member := range info.Members.MemberMap {
userIDs = append(userIDs, member.Sender)
}
avatars := r.redditorAvatars(ctx, userIDs)
for _, member := range info.Members.MemberMap {
if avatar := avatars[member.Sender]; avatar != nil && member.UserInfo != nil {
member.UserInfo.Avatar = avatar
}
}
return info
}

func downloadRedditorAvatar(ctx context.Context, url string) ([]byte, error) {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return nil, err
}
resp, err := avatarHTTPClient.Do(req)
if err != nil {
return nil, mediaError(ctx, "Reddit avatar download", err)
}
defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("reddit avatar is unavailable (HTTP %d)", resp.StatusCode)
}
return readMediaBounded(resp.Body, 2*1024*1024)
}
19 changes: 5 additions & 14 deletions pkg/connector/chatinfo.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func (r *RedditClient) GetChatInfo(ctx context.Context, portal *bridgev2.Portal)
if key != portal.PortalKey {
return nil, errors.New("chat ownership does not match Reddit room state")
}
return r.chatInfoFromState(state), nil
return r.addMemberAvatars(ctx, r.chatInfoFromState(state)), nil
}

// GetUserInfo fetches profile info for a ghost (a Reddit user puppeted into
Expand All @@ -40,19 +40,10 @@ func (r *RedditClient) GetUserInfo(ctx context.Context, ghost *bridgev2.Ghost) (
Username: stripUserPrefix(name),
UserID: string(ghost.ID),
})
info := &bridgev2.UserInfo{Name: ptr.Ptr(formatted)}
if !resp.AvatarURL.IsEmpty() {
mxc := resp.AvatarURL.CUString()
info.Avatar = &bridgev2.Avatar{
ID: networkid.AvatarID(mxc),
Get: func(ctx context.Context) ([]byte, error) { return r.downloadRedditMedia(ctx, mxc, nil) },
}
if r.main.directMedia {
// Retain the normal download fallback if the identity cannot fit.
info.Avatar.MXC, _ = r.directMediaURI(ctx, mxc)
}
}
return info, nil
return &bridgev2.UserInfo{
Name: ptr.Ptr(formatted),
Avatar: r.redditorAvatars(ctx, []networkid.UserID{ghost.ID})[ghost.ID],
}, nil
}

func stripUserPrefix(name string) string {
Expand Down
4 changes: 2 additions & 2 deletions pkg/connector/chatsync.go
Original file line number Diff line number Diff line change
Expand Up @@ -329,9 +329,9 @@ func (r *RedditClient) joinedRoomEvent(state *RoomState, key networkid.PortalKey
info := r.chatInfoFromState(state)
return &simplevent.ChatResync{
EventMeta: simplevent.EventMeta{Type: bridgev2.RemoteEventChatResync, PortalKey: key, CreatePortal: true},
GetChatInfoFunc: func(context.Context, *bridgev2.Portal) (*bridgev2.ChatInfo, error) {
GetChatInfoFunc: func(ctx context.Context, _ *bridgev2.Portal) (*bridgev2.ChatInfo, error) {
// Account block state may change while this room snapshot is queued.
return r.chatInfoFromState(state), nil
return r.addMemberAvatars(ctx, r.chatInfoFromState(state)), nil
},
CheckNeedsBackfillFunc: func(_ context.Context, latest *database.Message) (bool, error) {
return info.CanBackfill && (limited || latest == nil), nil
Expand Down
5 changes: 5 additions & 0 deletions pkg/connector/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ type RedditClient struct {

typingMu sync.Mutex
typing map[networkid.PortalKey][]id.UserID

avatarMu sync.Mutex
avatarURLs map[networkid.UserID]string
avatarFetching map[networkid.UserID]bool
avatarRetryAt time.Time
}

var _ bridgev2.NetworkAPI = (*RedditClient)(nil)
Expand Down
4 changes: 2 additions & 2 deletions pkg/connector/requests.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,8 @@ func (r *RedditClient) pendingRequestEvent(state *RoomState, key networkid.Porta
info := r.chatInfoFromState(state)
return &simplevent.ChatResync{
EventMeta: simplevent.EventMeta{Type: bridgev2.RemoteEventChatResync, PortalKey: key, CreatePortal: true},
GetChatInfoFunc: func(context.Context, *bridgev2.Portal) (*bridgev2.ChatInfo, error) {
return r.chatInfoFromState(state), nil
GetChatInfoFunc: func(ctx context.Context, _ *bridgev2.Portal) (*bridgev2.ChatInfo, error) {
return r.addMemberAvatars(ctx, r.chatInfoFromState(state)), nil
},
CheckNeedsBackfillFunc: func(_ context.Context, latest *database.Message) (bool, error) {
// Populate an existing empty request room too. Subsequent resyncs
Expand Down
27 changes: 27 additions & 0 deletions pkg/redditchat/redditors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package redditchat

import "context"

// GetRedditorIcons returns avatar URLs keyed by t2_ ID. Reddit's chat Matrix
// server has no avatars; the integrated chat UI reads them from here.
func (s *RedditSession) GetRedditorIcons(ctx context.Context, redditorIDs []string) (map[string]string, error) {
var data struct {
RedditorsInfoByIDs []struct {
ID string `json:"id"`
Icon *struct {
URL string `json:"url"`
} `json:"icon"`
} `json:"redditorsInfoByIds"`
}
err := s.chatGraphQL(ctx, "ChatRedditors", map[string]any{"ids": redditorIDs, "includeRedditorAttributes": true}, &data)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪️ Nit · Missing redditorsInfoByIds caches every requested user as having no avatar

If the response has data but no redditorsInfoByIds list (schema change, null field, different shape), this returns an empty map with no error. redditorAvatars then caches "" for every ID in the batch for the client's lifetime, so those ghosts get no avatar until restart. GetBlockedUsers in blocking.go treats an absent list as an error for this reason; do the same here.

Found by Indent Review Agent

if err != nil {
return nil, err
}
icons := make(map[string]string, len(data.RedditorsInfoByIDs))
for _, redditor := range data.RedditorsInfoByIDs {
if redditor.Icon != nil && redditor.Icon.URL != "" {
icons[redditor.ID] = redditor.Icon.URL
}
}
return icons, nil
}
Loading