tangled.org / core
Monorepo for Tangled tangled.org
fork atom

show handles of users that reacted to issues and pulls #636

merged opened by camsmith.dev targeting master from camsmith.dev/core: feat/user-reaction-handles

Displays the handles of users that reacted on issues and pulls, in addition to the counts. Closes https://tangled.org/@tangled.org/core/issues/246

There's a couple ways to do it

  1. Doing the db query in 1 roundtrip -- that's what I did here
  2. Doing 2 db queries, 1 for the count and 1 for the usernames

If we prefer (2), or another route, that's fine, I can change it.

Labels

None yet.

assignee

None yet.

Participants 3
AT URI
at://did:plc:5pio6u2p274ls56agcm7nz3q/sh.tangled.repo.pull/3m2izmuycuf22
+18 -18
Interdiff #1 → #2
+2 -4
appview/db/reaction.go
··· 62 62 return count, nil 63 63 } 64 64 65 - func GetReactionMap(e Execer, threadAt syntax.ATURI) (map[models.ReactionKind]models.ReactionDisplayData, error) { 66 - const maxUsersPerKind = 20 67 - 65 + func GetReactionMap(e Execer, userLimit int, threadAt syntax.ATURI) (map[models.ReactionKind]models.ReactionDisplayData, error) { 68 66 query := ` 69 67 select kind, reacted_by_did, 70 68 row_number() over (partition by kind order by created asc) as rn, ··· 94 92 95 93 data := reactionMap[kind] 96 94 data.Count = total 97 - if rn <= maxUsersPerKind { 95 + if userLimit > 0 && rn <= userLimit { 98 96 data.Users = append(data.Users, did) 99 97 } 100 98 reactionMap[kind] = data
+1 -1
appview/issues/issues.go
··· 83 83 return 84 84 } 85 85 86 - reactionMap, err := db.GetReactionMap(rp.db, issue.AtUri()) 86 + reactionMap, err := db.GetReactionMap(rp.db, 20, issue.AtUri()) 87 87 if err != nil { 88 88 l.Error("failed to get issue reactions", "err", err) 89 89 }
appview/models/reaction.go

This file has not been changed.

+1
appview/pages/pages.go
··· 1010 1010 ThreadAt syntax.ATURI 1011 1011 Kind models.ReactionKind 1012 1012 Count int 1013 + Users []string 1013 1014 IsReacted bool 1014 1015 } 1015 1016
+5 -6
appview/pages/templates/repo/fragments/reaction.html
··· 20 20 dark:hover:border-gray-600 21 21 {{ end }} 22 22 " 23 + {{ if gt (length .Users) 0 }} 24 + title="{{ range $i, $did := .Users }}{{ if ne $i 0 }}, {{ end }}{{ resolve $did }}{{ end }}{{ if gt .Count (length .Users) }}, and {{ sub .Count (length .Users) }} more{{ end }}" 25 + {{ else }} 26 + title="{{ .Kind }}" 27 + {{ end }} 23 28 {{ if .IsReacted }} 24 29 hx-delete="/react?subject={{ .ThreadAt }}&kind={{ .Kind }}" 25 30 {{ else }} ··· 30 35 hx-disabled-elt="this" 31 36 > 32 37 <span>{{ .Kind }}</span> <span>{{ .Count }}</span> 33 - {{ if gt (length .Users) 0 }} 34 - <div class="absolute bottom-full left-1/2 -translate-x-1/2 mb-2 px-3 py-2 bg-gray-900 dark:bg-gray-700 text-white text-sm rounded shadow-lg opacity-0 invisible group-hover:opacity-100 group-hover:visible transition-opacity pointer-events-none w-96 break-words z-10"> 35 - {{ range $i, $did := .Users }}{{ if $i }}, {{ end }}{{ resolve $did }}{{ end }}{{ if gt .Count (length .Users) }}, and {{ sub .Count (length .Users) }} more{{ end }} 36 - <div class="absolute top-full left-1/2 -translate-x-1/2 -mt-1 border-4 border-transparent border-t-gray-900 dark:border-t-gray-700"></div> 37 - </div> 38 - {{ end }} 39 38 </button> 40 39 {{ end }}
appview/pages/templates/repo/issues/issue.html

This file has not been changed.

appview/pages/templates/repo/pulls/fragments/pullHeader.html

This file has not been changed.

+1 -1
appview/pulls/pulls.go
··· 189 189 m[p.Sha] = p 190 190 } 191 191 192 - reactionMap, err := db.GetReactionMap(s.db, pull.PullAt()) 192 + reactionMap, err := db.GetReactionMap(s.db, 20, pull.PullAt()) 193 193 if err != nil { 194 194 log.Println("failed to get pull reactions") 195 195 s.pages.Notice(w, "pulls", "Failed to load pull. Try again later.")
+8 -6
appview/state/reaction.go
··· 70 70 return 71 71 } 72 72 73 - count, err := db.GetReactionCount(s.db, subjectUri, reactionKind) 73 + reactionMap, err := db.GetReactionMap(s.db, 20, subjectUri) 74 74 if err != nil { 75 - log.Println("failed to get reaction count for ", subjectUri) 75 + log.Println("failed to get reactions for ", subjectUri) 76 76 } 77 77 78 78 log.Println("created atproto record: ", resp.Uri) ··· 80 80 s.pages.ThreadReactionFragment(w, pages.ThreadReactionFragmentParams{ 81 81 ThreadAt: subjectUri, 82 82 Kind: reactionKind, 83 - Count: count, 83 + Count: reactionMap[reactionKind].Count, 84 + Users: reactionMap[reactionKind].Users, 84 85 IsReacted: true, 85 86 }) 86 87 ··· 109 110 // this is not an issue, the firehose event might have already done this 110 111 } 111 112 112 - count, err := db.GetReactionCount(s.db, subjectUri, reactionKind) 113 + reactionMap, err := db.GetReactionMap(s.db, 20, subjectUri) 113 114 if err != nil { 114 - log.Println("failed to get reaction count for ", subjectUri) 115 + log.Println("failed to get reactions for ", subjectUri) 115 116 return 116 117 } 117 118 118 119 s.pages.ThreadReactionFragment(w, pages.ThreadReactionFragmentParams{ 119 120 ThreadAt: subjectUri, 120 121 Kind: reactionKind, 121 - Count: count, 122 + Count: reactionMap[reactionKind].Count, 123 + Users: reactionMap[reactionKind].Users, 122 124 IsReacted: false, 123 125 }) 124 126

History

3 rounds 7 comments
sign up or login to add to the discussion
camsmith.dev submitted #2
diff
1 commit
expand
02fca247
appview/{db,pages,models}: show tooltips for user handles when hovering on reactions
expand 1 comment
oppi.li

awesome, thanks for the PR! works like a charm.

pull request successfully merged
camsmith.dev submitted #1
diff interdiff
1 commit
expand
5dc1e832
appview/{db,pages,models}: show tooltips for user handles when hovering on reactions
expand 6 comments
camsmith.dev

I chose an arbitrary cutoff of 20 usernames to display

boltless.me

I have a different implementation separating username resolving from service layer. I'll open a separate PR soon

boltless.me

Actually nvm. This seems fine.

camsmith.dev

The benefit of (2) being that if there are a lot of reacts, we aren't querying all of them from the db. so it's probably preferable to make two separate small db calls than 1 unbounded one

oppi.li

thanks for the PR! this works like a charm, couple of nits:

  • can we avoid hardcoding maxUsersPerKind in the db module? we can just return all the users here or accept a limit parameter.
  • instead of a custom tooltip, can we use the title attribute? this has the added benefit of being accessible! something like this might work:
        {{ if gt (length .Users) 0 }}
          title="{{ range $i, $did := .Users }}{{ if ne $i 0 }},{{end}} {{ resolve . }}{{ end }}"
        {{ else }}
          title="{{ .Kind }}"
        {{ end }}
  • lastly, the the repo/fragments/reaction fragment is broken in two scenarios: when it is rendered from either of the endpoints in state/reaction.go, this is because Users is not passed in. the rendering itself is fine, but there is a latent error in rendering the template. hovering over a reaction right after you react will not show the tooltip. a refresh will be necessary.
camsmith.dev

Thank you for the review @opp.li ! Addressed, I believe

camsmith.dev submitted #0
diff
1 commit
expand
5892994a
appview/{db,pages,models}: show tooltips for user handles when hovering on reactions
expand 0 comments