Show reviewer avatars and highlight member ACKs in summary comment#72
Show reviewer avatars and highlight member ACKs in summary comment#72l0rinc wants to merge 3 commits intomaflcko:mainfrom
Conversation
Keep review table entries as `Review` while grouping/sorting by AckType, and centralize reviewer markdown formatting in `format_reviewer_link()`. Add a unit test that locks in the current output to make follow-up presentation changes (avatars/member highlighting) safer.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. |
|
Seems fine, if other maintainers want this, but I think there are a few concerns:
|
|
All reviews will still be used, it's just a lot more obvious who did the (n)acking (pic + bold for members). |
|
Maybe ask in the dev meeting for input? I am -0, but I don't want to pull the trigger here one way or the other myself. |
|
Concept ~ How about showing a picture? |
Thread each reviewer avatar URL through parsed comments/reviews and render a small inline avatar next to their name in the summary table. The avatar request size (`s=14`) is based on GitHub `TimelineItem` inline avatars - but it was not explicitly tested.
l0rinc
force-pushed
the
l0rinc/ack-avatar-member
branch
from
338ecb5 to
d1ab647
Compare
|
Updated the avatar sizes to be smaller and removed the link from the avatar, keeping it only on the name (looks better), see: https://gist.github.com/l0rinc/6e92f6ac500a7ab9cc91d0b19a009392 |
|
I think I'm concept ack, approach ~0. I don't care much for the low-effort ACKs at the moment, that doesn't seem big enough of a problem yet, and we can fix when it's relevant / when maintainers find it an issue. I think impersonation is a much more dangerous potential problem, and although I haven't seen instances of it yet, it's better to protect before it happens. Using user profile images seems to add little benefit because of how easily spoofable it is, plus it might be confusing since users can change their profile pictures at will. Perhaps using something like https://lifehash.info/ would be a bit more robust? Then again, if we want to keep the image size small, it's not going to be terribly hard to bruteforce a similar-enough picture. Although I don't like the differentiation between members and non-members for the same reason as outlined here, it does provide a pretty straightforward and non-invasive way of doing that. Another less intrusive alternative would be to add a warning emoji after non-members' usernames if their similarity (not sure if this can be calculated robustly) with a member username exceeds a certain threshold. This might require adding state to DrahtBot to e.g. exclude known collisions, though. |
|
+1 on the change adding the icons. I think the icons in the screenshot look nice and would make the GitHub page easier to navigate, making the draftbot comment stand out from the wall of plain text and making individual review links easier to find. I also think the color and goofiness of the display send a better message: this is not an official tally of votes, it's a social display of which people participated in discussion of an issue. I don't think highlighting member acks is a good idea though, and would oppose that change. Maintainers do weigh reviews differently based on reviewer's history and perceived familiarity with area of the code being modified, but GitHub affiliation isn't really a factor in this and IMO highlighting it would be a distraction and could be source of confusion. One change I'd really like independently of the other ideas is a link to past acks. Like I'd love if drahtbot displayed "ryanofsky [0] [1] [2]" where "ryanofsky" links to the final ack like currently but [0] [1] [2] link to earlier acks. Normally when I review a PR, I write my full feedback in the first ack and and then incrementally reack after that. But in current GitHub UI it can be difficult to find previous acks and having direct links would make it much easier. |
Heh, that is an interesting view. Personally, I'd say it will be harder to follow review links for outsiders, because putting the user image next to the user name may indicate that the link it points to is the user profile (at least for most of the remainder of github, this is the case). An alternative to make the table more goofy, could be to ask an LLM to add emojis to the left column 😅 . |
|
Yeah I should clarify I think the table has two purposes: (1) - Making the GitHub page navigable by providing direct links to code reviews. I think (1) is more important than (2) by far. But the icons could help with both, helping find individual links more easily and helping see at a glance who participated in a review. I also think the colorful appearance of the icons could help dispel the misconception that that the table is a highly official list of votes, but this would be like a third or fourth order effect |
Present reviewer names in italics when GitHub reports `AuthorAssociation::Member`, so member ACKs stand out in the summary table.
l0rinc
force-pushed
the
l0rinc/ack-avatar-member
branch
from
d1ab647 to
8d19a96
Compare
|
Concept ~ This would be a valuable idea if you were using your own issue tracker and you could show a big pic on name hovering, in a temporary tool-tip. Because you are using GitHub, you need to show a permanent pic, and showing a small pic next to the name is better than nothing, but it's almost same as nothing. It is a great idea for a different context. |
I think impersonation will likely be caught quickly, if it ever happens, or at least this change won't help detect it quicker, because:
Maybe I can use this opportunity to ask reviewers to sign their comments. It is no harder than fetching the code (which any serious reviewer will have to do either way), and a dedicated (sub)signing key can be set up with a one-time cost. Finally, a bash alias or function can make it usable: |
| users | ||
| .iter() | ||
| .map(|(user, url, _)| format!("[{user}]({url})")) | ||
| .map(format_reviewer_link) |
There was a problem hiding this comment.
nit: Seems a bit odd to extract this for a unit test on a single format call
| body: c.body.unwrap_or_default(), | ||
| date: c.updated_at.unwrap_or(c.created_at), | ||
| .map(|c| { | ||
| let user = c.user; |
| body: c.body.unwrap_or_default(), | ||
| date: c.submitted_at.unwrap(), | ||
| .map(|c| { | ||
| let user = c.user.unwrap(); |
There was a problem hiding this comment.
I couldn't unwrap it multiple times (the previous was just extracted for symmetry)
|
Updated the code to show members in italics instead of bold - if this is still too much, I'll drop it completely.
Working on it, I like the idea. Maybe even a mousover summary of the review (either static or LLM generated) |
|
Thanks for the feedback in https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2026-01-08. |
Added something similar to #74, thanks for the hint |
6 participants
Context
ACKsignals are easy to game via sockpuppets / low-effort accounts: names alone don’t provide much friction against impersonation or ACK stuffing.Fix
Adding avatars increases the difficulty slightly of blending in (visual identity, easier spotting of suspicious patterns), and highlighting members helps reviewers quickly weight ACKs from people with established repo standing, see: https://gist.github.com/l0rinc/6e92f6ac500a7ab9cc91d0b19a009392
Note
Tried different avatar sizes, ended up with this size.