Page MenuHomePhabricator

Red "X" for requesting changes on code reviews is rather harsh
Closed, WontfixPublic


Some engineers on our team are reluctant to "request changes" in Differential because the red "X" seems too harsh if the suggested changes are minor. Leaving feedback as mere comments doesn't give us the same workflow, though โ€“ the change still shows on the reviewer's dashboard as "waiting for review".

Any chance we could get an icon that is fuzzy and cheerful and represents change?

Event Timeline

ohler raised the priority of this task from to Needs Triage.
ohler updated the task description. (Show Details)
ohler added a subscriber: ohler.

We don't plan to pursue this in the upstream.

chad claimed this task.

Also, try out the meme generator.


FYI, since you specifically mention upstream โ€“ we're using Phacility, so we
can't use a fork, either.

We use to represent the states of each reviewer in Differential. These icons make it fairly clear the status of each, even if you've never used the product before, we also use these same icon patterns in other areas of the product. As for colors we have standard patterns we prefer to adhere to, red (stop, important), yellow (warning), white (neutral), blue (notice), green (success). We also use other colors as consistently as we can as well like purple (closed) and sky (highlight). A consistent design pattern means people understand the UI quicker and better.

While there may be other icons to choose from, they'll all need to perform the same base function, which is to explain that changes are needed before this revision will be accepted. My general assumption here is that change an icon or color will not resolve anything with your engineer's workflow, since they mean the same thing inside the product. That is, this code needs updating as it will break something (reject).

Most of these issues I think can be resolved with Dashboards (ie, people can build whatever they want to track in Differential), T418 (maybe make reviewer status part of the "card"), and through corporate culture (discussing best code review practices). For me, if a reviewer doesn't reject, but I need to follow up, I just "plan changes" to keep it up higher in my queue. I also don't tend to work on more than a handful of diffs at a time, so knowing the state of what's out there is easier (for me). If you have more specifics as to why it's difficult to track your diffs, we can look and provide feedback on workflows.

@epriestley I think once mentioned the concept of queues and moving diffs back and forth. Not sure if he's still thinking about things like that, but resolving someone worried about 'Requesting Changes' would likely be better solved by re-thinking how we pass reviews back and forth from top down.

I was planning to try make "comment" work somewhat like a "soft reject" at some point, but this was before T1279. After T1279, I haven't seen much feedback to the effect that this is important (I think essentially all feedback since T1279 -- other than this case -- is related to cases like the one discussed in T8080 -- roughly, revisions appear in a bucket based on the overall revision state right now, but viewers expect them to appear in a bucket based on their personal reviewer state).

To the degree that "reject" is a problem because it's too harsh, I don't think it can be remedied by making the icon softer. This was more of a problem before T1279 (when we introduced icons) and previous softening of the language prior to 2010 (from "Reject" to "Request Changes") didn't seem to have much effect.

If we do pursue something here, I still favor making "comment" work like "soft reject" as the first / most promising approach, although this is substantially more complicated after T1279 when we have to consider the state of all the reviewers. I haven't though through the cases and I'm not sure an implementation is available which could work as well as it once could have, even in theory. My second-favorite approach is something like removing the "Comment" action for reviewers entirely, or making it non-default, but I suspect many users would find this frustrating and hate it.

Ultimately, I think incentives are fairly well-aligned here even when reviewers don't "reject". The revision author needs to work slightly harder to keep track of their in-flight code, but as long as "Comment" is a possible action they'll always have to do this, and it's generally desirable for the author to bear the overhead here -- they're ultimately responsible for the change.

Overall, my current thinking is that I'll address more clear-cut bucketing cases which arose after T1279 (for example, T8080) first, and then consider making comments push things between buckets (and possibly introduce new buckets for this, like an "Unreplied Comments" bucket between "Blocking Others" and "Waiting on Others").

Tentatively, the bucketing rules might change to:

  • Blocking Others: Revisions in a "Needs Review" state where you are not the author, and you or a project you are a member of is a reviewer and at least one of the reviewers you have authority over is not in a "Request Changes" or "Accept" state, or some of the reviewers you have authority over are in conflicting states.
  • Unreplied Comments: Revisions which you are the author of in a "Needs Review" state with comments since the last deliberate action you took on the revision.
  • Action Required: Revisions you are the author of in a "Needs Changes" or "Accepted" state.
  • Waiting on Others: Revisions in a "Needs Review" state which you are the author of which don't have comments since your last deliberate action, or revisions you have reviewer authority for in a "Needs Changes" or "Accepted" state, where all of the reviewers you have authority over are in the same state.

I think this is probably reasonable overall, but there are some challenges:

  • It's significantly more expensive to compute this bucketing, and the bucketing we do to generate the homepage count is already unreasonably expensive; see T7707. That should happen first, at a minimum.
  • For the "Unreplied Comments" bucket, we don't have a formal concept of a "deliberate action" and it's likely to be fuzzy no matter how we try to define it.
  • For the "Unreplied Comments" bucket, there are likely cases where, e.g., bots leave comments. Ignoring bots might produce better results, but is more complex/expensive and makes the rule harder to understand. Bot comments also probably are not unambiguously meaningless.
  • Introducing an "Unreplied Comments" bucket will create an incentive for authors to leave garbage comments in some cases in order to push things off their queue. This may be a strong incentive if it isn't clear to authors that the buckets are asymmetrical and the revision is still in "Blocking Others" for their reviewers.
  • Revision state is complicated and I've probably forgotten some cases.

This is likely to happen roughly as: fixing T7707, collecting cases like T8080, applying the new viewer-revision-state-based bucketing rules (only -- no unreplied comments bucket yet) to resolve these cases if none of them are problematic, letting that sit for a while to be confident that moving bucketing rules to be more-asymmetric is really a net win and not unduly confusing, then considering adding the "Unreplied Comments" bucket.