Yeah, this feels a little cluttered. We're already give two levels of information here to the user, "the bucket" and "the status". Individual status seems unimportant, maybe? Do I need to know my status for it (probably not?)? The only thing I personally might want to know is who set the current status. That is, if it's "Accepted" show that user (or users if also blocked) with the icon. I don't think others matter in a summary view?

The screenshot isn't very good since I don't have a bunch of package/project stuff set up locally, but the value of this is primarily for project/package reviewers. A useful version of this is something like:

Reviewers: [(X) Security], (X) UIE, (√) Platform Eng

Particularly, the use cases I think this is most valuable for are:

• Distinguish between things in your "Must Review" bucket and "Should Review" bucket that are there because you have to review them, vs there because some package or project you have authority over has to review them ("Is this for me personally, or just a platform eng revision?")
• Show which projects/packages you have authority over ("Why do I have to review this?").
• Show status of projects/packages ("Why is this blocked on me?").

All of this is pretty useless with user reviewers. We could only show it for project/package reviewers, but that feels weirdly inconsistent to me?

Basically, this is mostly about getting more information on "Must Review" and "Should Review" at a glance, in situations with package/project reviewers, and reducing our need to have some kind of "Why am I a reviewer for this?" feature. Knowing who put a revision into the current state isn't useful for those cases (it's always the author).

T10689 was a specific request in this vein, and I think there were a couple of others, although everything related to this has been merged 20 times so I'm having trouble digging it up immediately.

Of course, it's possible we don't need to go this far and that the more granular bucketing alone is sufficient, or that we could do bucketing + authority only to split the difference here. Or only show icons for projects/packages. Or divide this into "Authority Reviewers" and "Other Reviewers" with no coloring. But all of these have less information in the "Why is this stuff in my must review / should review buckets?" use case than the full icons + highlights does.

(And T10689 was really "All Reviewers Must Accept" anyway. I think we could safely hold this and wait for feedback, at a minimum, if you aren't convinced about the merits. This might reasonably be overkill, and many of the problems in this vein should be at least mitigated by the new bucketing.)

I'm tepid right now. Maybe hold and see how things go? I'm a-ok on removing the redundant text though.

Sounds good. I'll move the redundant text removal into D15927 since I think that's a clear win too.

In D15926#185059, @epriestley wrote:

Particularly, the use cases I think this is most valuable for are:

• Distinguish between things in your "Must Review" bucket and "Should Review" bucket that are there because you have to review them, vs there because some package or project you have authority over has to review them ("Is this for me personally, or just a platform eng revision?")
• Show which projects/packages you have authority over ("Why do I have to review this?").
• Show status of projects/packages ("Why is this blocked on me?").

FWIW, all three of these use cases resonate with our extensive use of owner packages, especially when owner packages are particularly large.

Along the lines of use case 1, a large team in particular opted to create their own custom dashboard because they found the "must review" bucket really noisy and hard to distinguish which revisions were ones for them personally. A few other teams have expressed confusion about why certain revisions are in the "must review" bucket, so any sort of context about projects / packages would be helpful there.

Re: use case 3, we've had a handful of people request to know what revisions they've commented on (not just requested changes / accepted for), to get a better idea of which revisions they haven't looked at yet.

Any plans to revive this work?

I don't think we currently have any plans to revisit this change, specifically.

For (1), without this change, you can write a query for "Responsible Users: exact(your-username)" and save that as "Stuff I, Specifically, Have Been Asked To Review", or just "Reviewers: your-username, Status: Needs Review, No Bucketing". Is that roughly what users are doing with custom dashboards? Is it unclear that they can do this?

For (3) we could conceivably repurpose the "draft" bubble and show a different version of it if you've commented (e.g., yellow = draft, blue = submitted comment) although this change doesn't immediately resonate with me.

If users don't remember if they've interacted with a change or not, I wonder if maybe automatic reviewer rules are just getting a bit out of control? At Facebook, there was a period where I remember getting some similar feedback about difficulty keeping track of why users were reviewing different changes. In response, I removed the ability to use Herald to add reviewers. This appeared to substantially solve the problem without creating new problems: it seemed like a lot of users had written rules they didn't really need and were sort of getting overwhelmed by their effects.

The bucketing is also modular now and you could possibly copy/paste the modern bucket and add a "You, Personally, Must Review" bucket on top of "Must Review". But if you need to do this, I wonder if users really need to be part of so many blocking reviewer groups?

One tangental change which I think we're likely to do is let you install global defaults alongside the personal defaults in query UIs, which would potentially let you write a "Needs Personal Review" filter and make it available globally. To the degree that this is a configuration/administration issue (e.g., not obvious to users how to use filters, or they're finding it cumbersome to go through all the steps to build queries even though they know how) that might reduce the burden.

I also think "teams are building dashboards to manage their workflows" is basically a desirable outcome, not necessarily an indicator that this UI isn't serving users well. If this UI covers 95% of users and the other 5% use dashboards to solve their problems, that's roughly what we're hoping for in product design. If we try to take this UI from serving 95% of use cases to serving 99%, it may work better for 4% of users and worse for the other 95% who didn't need all the extra clutter. This particular change definitely made the dashboard a good deal more cluttered.

Thanks for the feedback. There is definitely a problem in general where people are added to too many reviews, but it's generally due to being part of too many owner packages. We have been encouraging teams to prune their ownership to minimize gatekeeping, but it's still helpful to have targeted dashboards.

For (1), without this change, you can write a query for "Responsible Users: exact(your-username)" and save that as "Stuff I, Specifically, Have Been Asked To Review", or just "Reviewers: your-username, Status: Needs Review, No Bucketing". Is that roughly what users are doing with custom dashboards? Is it unclear that they can do this?

So, they are doing something along these lines, but they're creating a generic / shareable dashboard for their team. To make it more easily shareable (along the lines of that tangental change you had in mind), would it be possible to do exact(Current Viewer)?

would it be possible to do exact(Current Viewer)?

Yeah -- some discussion in T9430#214718 and maybe T12371, although I don't think there's a specific task for exactviewer() right now.

The short version is that I'm not entirely sure if we should just implement exactviewer() or try to implement exact(viewer()). We currently have viewerprojects() but it doesn't feel great and perhaps this should become projects(viewer()), but this is a significantly more involved change. If we go the simple route, T12371 implies we may start accumulating a lot of permutations of f(g(h())) as fgh(), e.g. viewerpackages(). If that path ends with us implementing a whole array of notviewerprojects(), anyviewerpackage(), etc., it's probably not the best path we could pick to walk down.

Yeah, I could see that being a not very elegant path to go down. I guess it depends on how difficult it is to implement f(g(h())). If it is a deep rabbit hole, then it's probably best to just fill in a few specific use cases that we think are compelling, e.g. exactviewer() (assuming you think it's useful).