7 ms·

This is cool! I'm happy to see more options in this space. The best code review tool I've ever used was a tool at Google called Critique.[0] They've open-sourc

This is cool! I'm happy to see more options in this space. The best code review tool I've ever used was a tool at Google called Critique.[0] They've open-sourced it as Gerrit[1], but there are sadly no hosted versions available for under $15k/yr, and it's complicated to self-host. I've been using Reviewable, and my experience has been good not great. Github's native code review has caught up a bit, but Github's review tool falls apart if your review lasts more than one round. Here are my gripes with Reviewable: Steep learning curve - Every new developer who joins the team spends their first few reviews being confused and frustrated by Reviewable. Performance - Reviewable has awful performance. It takes about 10 seconds to load a code review. It seems like it's doing some odd websockets stuff where sometimes my "connection" to Reviewable will drop and I can't add comments. I've never experienced this with any other web app. It's gotten better over the last few years, but it's still annoyingly frequent. Complicated configuration - I just want the reviewer to be able to hit an "LGTM" button to mark their approval. Reviewable's decision about when a PR is approved is based on this complicated function combining whether the reviewer typed the text ":lgtm", how many people looked at the review, whether they also hit the approve button. Each repo has its own configuration, and I can't make org-level changes without changing every repo one at a time. Excessive permissions - This might be a Github thing, but you can't grant Reviewable permissions to a particular private repo - you have to grant it permissions to all of your private repos. Several developers who join my team need to create a dedicated Github account to avoid exposing their other private repos to Reviewable. Thread state is unclear - The options are "discussing", "satisfied," "blocking," or "working," and it's not obvious who's supposed to move the thread to what state at what point. No development - I've been a paying customer of Reviewable for about 7 years, and I can remember only 1-2 minor features that have been added during that time. They haven't updated their blog[2] in 6 years, and they've never communicated with me as a paying customer to tell me anything they're doing. I checked out Crocodile, and it looks like it has potential. I'm not sure I'd pitch it to my team to switch yet. Here are some of my thoughts: * When do the reviewer's comments become visible to the author? One of the must-have features for me is that both author and reviwer(s) can prepare a set of notes, but they're not visible to anyone else until they hit "publish" to share them with the team. Sometimes I make comments in one spot, and then as I read more of the code, I revise a previous comment. If all my comments publish immediately, I can't revise comments like that. Github, Reviewable, and Gerrit all support a flow of preparing comments and then committing them in a separate step. * Crocodile touts the floating thread thing, and I've never used a tool that has it, but it doesn't seem better to me. Inline comments do break the flow, but floating comments actually cover up the code and prevent me from reading it. I see I can close threads, but I can't figure out how to get them back. * Being able to comment on character-level granularity is cool! * I think your thread state is better than Reviewable's, but I'd prefer an even simpler model where threads are either "open" or "resolved." When an author responds to a comment, the default action is to resolve it, but the author can override the default and leave it "open" if their comment is asking for clarification rather than declaring a fix. The reviewer can reopen a thread if they feel that the author has misunderstood the note. 95% of the time in my reviews, the reviewer makes a note and the author resolves it, so having a whole extra confirmation phase for that last 5% feels unnecessary when the reviewer can just reopen it instead. * Ditto for review state. The only two states I've ever needed for a code review are "pending approval" and "approved." I've never wanted to mark a PR as "rejected" unless it's just a spam submission from a stranger on an open-source repo, and even then, I'd close it from Github rather than my code review tool. The worst I'll do to a teammate is withhold approval until they address my notes, but I'd never mark it as "rejected." I don't need an explicit state for "pending review" or "waiting for author" because if the author is the last commenter, it's implicitly pending review. * I like that there's a view of all the comments at once. I like to review all my comments before pushing them to the author. * I'd like a way to mark a comment as "no action required" when I just want to say something nice[2] about the code that doesn't require action from the author. * I couldn't understand the "iterations" UI control. It's not obvious to me what the different circles represent.[4] Once I compared two diffs, I couldn't figure out how to compare to the the full PR to the base branch (i.e., all commits aggregated). I think it's replicating a control that Reviewable actually does pretty well, so I recommend giving it a look for inspiration. * It looks like I'm only allowed to make code-level comments, but I'd like to make review-level comments as well for high-level notes about the review as a whole. Hope that's useful. I'm very interested in code reviews, so if you want to do user interviews, feel free to reach out. You can find my contact info through my HN profile. [0] https://abseil.io/resources/swe-book/html/ch19.html https://abseil.io/resources/swe-book/html/ch19.html [1] https://www.gerritcodereview.com/ https://www.gerritcodereview.com/ [2] http://blog.reviewable.io/ http://blog.reviewable.io/ [3] https://mtlynch.io/human-code-reviews-2/#offer-sincere-praise https://mtlynch.io/human-code-reviews-2/#offer-sincere-prais... [4] https://i.imgur.com/3ZhDAR1.png https://i.imgur.com/3ZhDAR1.png

depr 4y ago
Have you used Review Board?
- mtlynch 4y ago
  No, I'd never heard of it. It looks interesting, but I'm kind of skeptical that a tool designed for reviewing images and source code can be as good as a tool that's dedicated to code review.
jameslao 4y ago
I can see how the iterations control can be not obvious. I'll add a docs section that explains it. The two columns of radio buttons represent your selection for the left and right side of the diff. You can also click on any iteration to diff it against the base. I've made a couple notes based on this to help me prioritize next. Thanks for taking the time to provide all this amazing feedback!
piotrkaminski 4y ago
Hey, thanks for the feedback on Reviewable! You make some really good points: > Steep learning curve Guilty as charged. I think it's cohesive and efficient once you learn it but the learning curve is quite steep. We keep trying to think of ways to flatten it a bit but haven't had any great ideas so far. If you have thoughts on this -- or could point us to a great UX designer with dev tools experience -- it would be much appreciated! > Performance There are some obvious things we're working towards to improve it but these days most reviews load in 3-4 seconds for me, even on a not-very-awesome laptop. What kind of platform are you seeing 10 second load times on, and how's your Internet latency to us-central? Please open an issue and we can work through it together. > Complicated configuration Yeah, there's a lot of support for legacy and highly customized workflows in there. But these days the primary and default integration is with GitHub's approve/request changes workflow, which gives you one-click approve -- have you tried using that? (Efficient multi-repo config updates are also on the todo list, but never quite rose to the top.) > Excessive permissions This is an unavoidable side-effect of using GitHub's OAuth authorization mechanism. GitHub Apps allow finer-grained permissions but they didn't exist when we created Reviewable and don't have a good answer for listing PRs, which would make our dashboard significantly less useful. We do have a transition planned out but at this point our main customers are enterprise folks who run their own GHE Server and don't much care about fine-grained permissions, so the ROI doesn't pencil out. > Thread state is unclear Huh, I'm surprised by this one. The basic flow is: 1. Reviewer creates a discussion (disposition defaults to Blocking). 2. Author responds with questions/comments (disposition defaults to Discussing). 3. Reviewer clarifies (disposition unchanged) or clicks the big Resolve button (disposition changes to Satisfied, discussion is resolved). 4. Author addresses the comment and clicks the big Done button (disposition changes to Satisfied). 5. Reviewer checks and, if satisfied, clicks Resolve (disposition changes to Satisfied and discussion is resolved). Basically, at every step you either reply to keep the discussion going, or click the button to indicate that you're fine with closing it out. You shouldn't even need to care about the specific disposition unless you're trying to run more advanced workflows. > No development Yeah, I pretty much abandoned the blog and should probably take it down. However, we do ship new features regularly and post updates on Headway [1]. These should also pop up as the "red circle new stuff counter" in the UI, but perhaps your browser is blocking that third-party connection. (Headway only gets major feature posts, but there's a lot more work going on in the background that's only reported on the enterprise changelog [2].) I'm not a fan of spammy email newsletters so we don't send those. Again, thanks for the feedback, and don't be a stranger -- it's easy to reach us through email, chat, issues, etc., and we respond promptly to every message. Even if we have to hunt them down on HN. :) [1] https://headwayapp.co/reviewable-changelog https://headwayapp.co/reviewable-changelog [2] https://github.com/Reviewable/Reviewable/blob/master/enterprise/changelog.md https://github.com/Reviewable/Reviewable/blob/master/enterpr...

depr 4y ago

Have you used Review Board?

mtlynch 4y ago
No, I'd never heard of it. It looks interesting, but I'm kind of skeptical that a tool designed for reviewing images and source code can be as good as a tool that's dedicated to code review.

jameslao 4y ago

I can see how the iterations control can be not obvious. I'll add a docs section that explains it. The two columns of radio buttons represent your selection for the left and right side of the diff. You can also click on any iteration to diff it against the base. I've made a couple notes based on this to help me prioritize next. Thanks for taking the time to provide all this amazing feedback!

piotrkaminski 4y ago

Hey, thanks for the feedback on Reviewable! You make some really good points: > Steep learning curve Guilty as charged. I think it's cohesive and efficient once you learn it but the learning curve is quite steep. We keep trying to think of ways to flatten it a bit but haven't had any great ideas so far. If you have thoughts on this -- or could point us to a great UX designer with dev tools experience -- it would be much appreciated! > Performance There are some obvious things we're working towards to improve it but these days most reviews load in 3-4 seconds for me, even on a not-very-awesome laptop. What kind of platform are you seeing 10 second load times on, and how's your Internet latency to us-central? Please open an issue and we can work through it together. > Complicated configuration Yeah, there's a lot of support for legacy and highly customized workflows in there. But these days the primary and default integration is with GitHub's approve/request changes workflow, which gives you one-click approve -- have you tried using that? (Efficient multi-repo config updates are also on the todo list, but never quite rose to the top.) > Excessive permissions This is an unavoidable side-effect of using GitHub's OAuth authorization mechanism. GitHub Apps allow finer-grained permissions but they didn't exist when we created Reviewable and don't have a good answer for listing PRs, which would make our dashboard significantly less useful. We do have a transition planned out but at this point our main customers are enterprise folks who run their own GHE Server and don't much care about fine-grained permissions, so the ROI doesn't pencil out. > Thread state is unclear Huh, I'm surprised by this one. The basic flow is: 1. Reviewer creates a discussion (disposition defaults to Blocking). 2. Author responds with questions/comments (disposition defaults to Discussing). 3. Reviewer clarifies (disposition unchanged) or clicks the big Resolve button (disposition changes to Satisfied, discussion is resolved). 4. Author addresses the comment and clicks the big Done button (disposition changes to Satisfied). 5. Reviewer checks and, if satisfied, clicks Resolve (disposition changes to Satisfied and discussion is resolved). Basically, at every step you either reply to keep the discussion going, or click the button to indicate that you're fine with closing it out. You shouldn't even need to care about the specific disposition unless you're trying to run more advanced workflows. > No development Yeah, I pretty much abandoned the blog and should probably take it down. However, we do ship new features regularly and post updates on Headway [1]. These should also pop up as the "red circle new stuff counter" in the UI, but perhaps your browser is blocking that third-party connection. (Headway only gets major feature posts, but there's a lot more work going on in the background that's only reported on the enterprise changelog [2].) I'm not a fan of spammy email newsletters so we don't send those. Again, thanks for the feedback, and don't be a stranger -- it's easy to reach us through email, chat, issues, etc., and we respond promptly to every message. Even if we have to hunt them down on HN. :) [1] https://headwayapp.co/reviewable-changelog https://headwayapp.co/reviewable-changelog [2] https://github.com/Reviewable/Reviewable/blob/master/enterprise/changelog.md https://github.com/Reviewable/Reviewable/blob/master/enterpr...