5 ms·

I’d settle for them making PRs as useful as they were in 2015, before they messed up some of the most basic functionality: showing the diff, and showing review

I’d settle for them making PRs as useful as they were in 2015, before they messed up some of the most basic functionality: showing the diff, and showing review comments. They hide big diffs behind a “load more” link, and as a result people often fail to code review the most substantial part of a change because they scan right past it, thinking it’s a removed file or binary or something. Then, once you submit a review, they only show 10 comments. In the middle, there’s an easy-to-miss “load more comments” button. These are the two most fundamental features of a PR. How could they decide so few as 10 is the right number of comments?

humanwhosits 5y ago
Hiding of the large files has tripped me up multiple times. I've starting having to look at the diffs outside github's UI
inetknght 5y ago
> They hide big diffs behind a “load more” link, and as a result people often fail to code review the most substantial part of a change because they scan right past it, thinking it’s a removed file or binary or something. This. Every PR I have to do ctrl-F "load diff" and then immediately click on _all_ of the diffs. It's !@#$ing annoying. I've also lost comments when the comment is part of a review and pushed to the PR after the the PR has been merged by someone else.
- corndoge 5y ago
  Might be worth making a bounty for it in refined github[0], similar things have been implemented in the past[1] [0] https://github.com/refined-github/refined-github https://github.com/refined-github/refined-github [1] https://github.com/refined-github/refined-github/issues/2151 https://github.com/refined-github/refined-github/issues/2151
  - inetknght 5y ago
    wtf is refined github? A browser extension? Nah, I value my browser more than that
    - judge2020 5y ago
      Especially for GitHub.com cookies and access. With over 50k users, this has to be a prime target for phishing/other attacks against the maintainers to publish a malicious update to the extension. Think passively stealing creds and source code.
  - metadat 5y ago
    An extension shouldn't be needed for a dev-centric service like GH to be usable. This is the wrong way to fight bad UX, as it's ridiculous to make installation of a potential security vulnerability of an addon necessary to make a git-frontend website work well. Better to just find a dev-first platform instead.
    - eloisius 5y ago
      Seriously. If they’re too busy making sketchy copyright decisions with their AI code generation to bother with basic usable UI, they don’t deserve to be the de facto home of open source. I’ve been giving sourcehut a try, and the simplicity is refreshing.
      - metadat 5y ago
        There's no denying the appeal of underdogs like SirHat :)
    - corndoge 5y ago
      Perhaps you are more fortunate than me, but in my career, there have been times where I couldn't dictate the platform used. This is actually the case for many people - I daresay the majority - who work in the software industry. So while I understand (and of course agree with) the sentiment, it's rather trite. Plugins are there for the rest of us.
- petepete 5y ago
  For the stuff I work on I prefer the big diffs to be hidden. It's nearly always yarn.lock and I have no desire to see that monstrosity in all its glory.
  - iovrthoughtthis 5y ago
    i have no idea why we include these in code reviews tbh
    - eloisius 5y ago
      Not saying I review dependency locks as well as I should, but one reason to is to prevent supply chain attacks. E.g. making a typo that installs a malicious package. I recently saw a $60k beast of a machine with 64 cores get pwnd. We all wondered why “-bash” was burning 48 cores of CPU until I attached strace to it and we saw JSON RPC messages indicative of crypto mining. Everyone with access to the machine is trustworthy, but someone may have typo’d a pip install or something like that.
  - MitchellCash 5y ago
    I think we can get the best of both worlds by hiding lock files by default and always showing any actual code file diffs. Performance may be a factor for very large diffs, but this would actually be my preference to how the default behaviour would work as I agree with you and the parent post.
    - petepete 5y ago
      A toggle in GitHub settings to automatically collapse `*.lock` files would be a massive step in the right direction.
88913527 5y ago
Maybe they're trying to nudge us towards smaller PRs. Many reviewers gloss over 300+ line changes in a single file. Approved with a "LGTM" and no further comment, but perhaps that's more a cultural issue with the team than anything.
- majormajor 5y ago
  While smaller PRs are better when practical/possible, the current Github design doesn't encourage them at all. If anything, it makes it too easy to submit giant ones - a bunch of the changes may get hidden anyway, making it look less intimidating than it should be!
- jcranmer 5y ago
  Perhaps, but github's PR UI goes to utterly atrocious if you try to do something along the lines of "a series of small atomic changes". In my recent reviewing experience, anything involving multiple commits in a single PR results in the diff more or less lying to you: you get diffs of something, but it's not clear what, and even less clear how to get what I want to actually diff. I think the evidence is pretty clear that the most effective development process for large codebases is to look at changes as effectively a series of patches applied to head-of-trunk (and those patches may evolve during review)... and github seems to almost go out of its way to prevent you from thinking about PRs as if they were patches.
  - deleted 5y ago
    [deleted]
  - thombles 5y ago
    > github seems to almost go out of its way to prevent you from thinking about PRs as if they were patches Those who take care to write meaningful per-commit descriptions are actively punished by the eager collapsing. Either reviewers have to (1) spot the ellipsis (2) click on it for each individual commit, or the submitter has to copy-paste all of their commit information into the PR description.
  - sdesol 5y ago
andrewf 5y ago
"Our metrics tell us that with these UI changes, people are approving more PRs and it's taking them less time!", probably.
thombles 5y ago
Additionally, the notification email links to "View it on GitHub" don't reliably cause the relevant parts of the page to expand so you end up wading through a huge PR expanding things at random until you find the message.
- eloisius 5y ago
  Exactly. Once they turned the page into JS framework soup instead of regular HTML, the anchor links don’t work worth a damn.
lachenmayer 5y ago
This is by far the most infuriating thing about the PR UI, along with the fact that you can't comment anywhere outside of the "patch", meaning you can't point out anything that is more than a couple of lines away.
- isbadawi 5y ago
  There's an item on their roadmap to allow commenting on unchanged lines: https://github.com/github/roadmap/issues/456 https://github.com/github/roadmap/issues/456
codeapprove 5y ago
formerly_proven 5y ago
It's still not as bad as BitBucket which makes it basically impossible to read commit messages in a pull request. Is that a good level to be at? Probably not. In the screenshot in the post it looks like the navigation tree is crammed into the 1280px wide grid, but that's not the case - enabling the feature preview makes the page full-width. So it doesn't make the diff area unreadable.

humanwhosits 5y ago

Hiding of the large files has tripped me up multiple times. I've starting having to look at the diffs outside github's UI

inetknght 5y ago

> They hide big diffs behind a “load more” link, and as a result people often fail to code review the most substantial part of a change because they scan right past it, thinking it’s a removed file or binary or something. This. Every PR I have to do ctrl-F "load diff" and then immediately click on _all_ of the diffs. It's !@#$ing annoying. I've also lost comments when the comment is part of a review and pushed to the PR after the the PR has been merged by someone else.

corndoge 5y ago
Might be worth making a bounty for it in refined github[0], similar things have been implemented in the past[1] [0] https://github.com/refined-github/refined-github https://github.com/refined-github/refined-github [1] https://github.com/refined-github/refined-github/issues/2151 https://github.com/refined-github/refined-github/issues/2151
- inetknght 5y ago
  wtf is refined github? A browser extension? Nah, I value my browser more than that
  - judge2020 5y ago
    Especially for GitHub.com cookies and access. With over 50k users, this has to be a prime target for phishing/other attacks against the maintainers to publish a malicious update to the extension. Think passively stealing creds and source code.
- metadat 5y ago
  An extension shouldn't be needed for a dev-centric service like GH to be usable. This is the wrong way to fight bad UX, as it's ridiculous to make installation of a potential security vulnerability of an addon necessary to make a git-frontend website work well. Better to just find a dev-first platform instead.
  - eloisius 5y ago
    Seriously. If they’re too busy making sketchy copyright decisions with their AI code generation to bother with basic usable UI, they don’t deserve to be the de facto home of open source. I’ve been giving sourcehut a try, and the simplicity is refreshing.
    - metadat 5y ago
      There's no denying the appeal of underdogs like SirHat :)
  - corndoge 5y ago
    Perhaps you are more fortunate than me, but in my career, there have been times where I couldn't dictate the platform used. This is actually the case for many people - I daresay the majority - who work in the software industry. So while I understand (and of course agree with) the sentiment, it's rather trite. Plugins are there for the rest of us.
petepete 5y ago
For the stuff I work on I prefer the big diffs to be hidden. It's nearly always yarn.lock and I have no desire to see that monstrosity in all its glory.
- iovrthoughtthis 5y ago
  i have no idea why we include these in code reviews tbh
  - eloisius 5y ago
    Not saying I review dependency locks as well as I should, but one reason to is to prevent supply chain attacks. E.g. making a typo that installs a malicious package. I recently saw a $60k beast of a machine with 64 cores get pwnd. We all wondered why “-bash” was burning 48 cores of CPU until I attached strace to it and we saw JSON RPC messages indicative of crypto mining. Everyone with access to the machine is trustworthy, but someone may have typo’d a pip install or something like that.
- MitchellCash 5y ago
  I think we can get the best of both worlds by hiding lock files by default and always showing any actual code file diffs. Performance may be a factor for very large diffs, but this would actually be my preference to how the default behaviour would work as I agree with you and the parent post.
  - petepete 5y ago
    A toggle in GitHub settings to automatically collapse `*.lock` files would be a massive step in the right direction.

88913527 5y ago

Maybe they're trying to nudge us towards smaller PRs. Many reviewers gloss over 300+ line changes in a single file. Approved with a "LGTM" and no further comment, but perhaps that's more a cultural issue with the team than anything.

majormajor 5y ago
While smaller PRs are better when practical/possible, the current Github design doesn't encourage them at all. If anything, it makes it too easy to submit giant ones - a bunch of the changes may get hidden anyway, making it look less intimidating than it should be!
jcranmer 5y ago
Perhaps, but github's PR UI goes to utterly atrocious if you try to do something along the lines of "a series of small atomic changes". In my recent reviewing experience, anything involving multiple commits in a single PR results in the diff more or less lying to you: you get diffs of something, but it's not clear what, and even less clear how to get what I want to actually diff. I think the evidence is pretty clear that the most effective development process for large codebases is to look at changes as effectively a series of patches applied to head-of-trunk (and those patches may evolve during review)... and github seems to almost go out of its way to prevent you from thinking about PRs as if they were patches.
- deleted 5y ago
  [deleted]
- thombles 5y ago
  > github seems to almost go out of its way to prevent you from thinking about PRs as if they were patches Those who take care to write meaningful per-commit descriptions are actively punished by the eager collapsing. Either reviewers have to (1) spot the ellipsis (2) click on it for each individual commit, or the submitter has to copy-paste all of their commit information into the PR description.
- sdesol 5y ago

andrewf 5y ago

"Our metrics tell us that with these UI changes, people are approving more PRs and it's taking them less time!", probably.

thombles 5y ago

Additionally, the notification email links to "View it on GitHub" don't reliably cause the relevant parts of the page to expand so you end up wading through a huge PR expanding things at random until you find the message.

eloisius 5y ago
Exactly. Once they turned the page into JS framework soup instead of regular HTML, the anchor links don’t work worth a damn.

lachenmayer 5y ago

This is by far the most infuriating thing about the PR UI, along with the fact that you can't comment anywhere outside of the "patch", meaning you can't point out anything that is more than a couple of lines away.

isbadawi 5y ago
There's an item on their roadmap to allow commenting on unchanged lines: https://github.com/github/roadmap/issues/456 https://github.com/github/roadmap/issues/456

codeapprove 5y ago

formerly_proven 5y ago

It's still not as bad as BitBucket which makes it basically impossible to read commit messages in a pull request. Is that a good level to be at? Probably not. In the screenshot in the post it looks like the navigation tree is crammed into the 1280px wide grid, but that's not the case - enabling the feature preview makes the page full-width. So it doesn't make the diff area unreadable.