Local PR review
At work I've been reviewing some large pull requests, ranging from thousands of lines of new code to tens of thousands, and in the process I've discovered how limiting GitHub's web interface for code review is, both the default UI and the beta UI. To handle these reviews, I started reviewing PRs on my local machine. Here's what my workflow looks like, with lots of links to NeoVim code and small scripts for manipulating diffs.
I briefly considered building my own web UI. I did that a few years ago when working on a team that hosted their code on BitBucket after I'd gotten used to GitHub reviews and treating review comments as notes to myself, coming back to delete questions answered by later code or clarifing thoughts before publishing anything. But before diving into a frontend application, I took a lesson from drafting pull requests locally and decided to see how far I could get with a diff, NeoVim, and some scripts.
A big advantage of reviewing code in a local text editor is that it can handle large diffs without requiring me to click "Load diff" on particular files or only showing one file at a time, which allows searching the whole set of changes for particular patterns, such as TODO comments or the catching of too broad exceptions. I can also exclude files in bulk, like generated code, documentation, and tests, depending on what my focus is for a given review. It's also a lot easier to review changes made since the last review, just by creating a diff with git diff <last-reviewed-commit>...HEAD.
But it's not a perfect experience, and I did a lot of customization to NeoVim to make it more diff-friendly. For starters, GitHub allows collapsing files. To support collapsing both files and chunks within a file, I defined a fold method and expression:
setlocal foldmethod=expr
setlocal foldexpr=v:lua.diff_fold_level()
setlocal foldtext=v:lua.diff_fold_text()
setlocal foldlevel=99
The Lua functions are implemented in Fennel:
(fn file-start? [line]
(string.match line "^diff")
(fn chunk-start? [line]
(string.match line "^@@"))
(fn _G.diff_fold_level [line]
(let [line (vim.fn.getline (or line vim.v.lnum))]
(if
(file-start? line) :>1
(chunk-start? line) :>2
:=)))
(fn _G.diff_fold_text []
(let [line (vim.fn.getline vim.v.foldstart)
count (+ 1 (- vim.v.foldend vim.v.foldstart))
level (_G.diff_fold_level vim.v.foldstart)
prefix (case level
(where :>1) ""
(where :>2) "▶ "
_ "")]
(.. prefix line " (" count " lines)")))
It's often helpful to jump from the diff to a particular file, to see the full context of the current code, so here's a command that uses the cursor's current position to find the right file name and chunk header, then calculate where to go and open it in a new tab.
Some chunks are not interesting and can be deleted from the diff, or whole files. Sometimes part of a chunk can be deleted but not others, so I also have a command to delete from the top of the chunk down to the cursor, which also updates the chunk header so jumping to the chunk's position in the file still works.
Often I start a review with the smallest changes, so here's a Babashka script that sorts the files in a diff by how many lines each file has. Using it for a while, I've added the ability to sort by how many additions each file has, or deletions, or how close the ratio of additions to deletions is, or whether the file name contains the word "test". And here's a script that moves the first file to the end of the diff, for cases where I don't want to delete the file but also don't want to review it yet.
Beyond shuffling code, I also needed to add comments. I opted to use markers in the first column of the dif, adding v and ^ to indicate what range of lines I'm commenting on, and x to indicate the end of a comment, like this:
diff --git a/old_file.txt b/new_file.txt
index 2a2e9f1..a48d7f5 100644
--- a/old_file.txt
+++ b/new_file.txt
@@ -1,1 +1,2 @@
-Notes on changes
+An ode
v
+To my code
^
👏
x
I add those markers with a couple of NeoVim commands that also put the highlighted lines into the copy register, which allows using them in an UltiSnips expansion, such as for making a suggestion. It's also trivial to adjust the range of lines being commented on by moving the markers, while to adjust the range of a comment on GitHub I have to copy a comment's content, delete the comment, select a new range, create a new comment, and paste the original comment.
I have a script to collect those comments as Markdown, with file and line metadata in the header of the fenced code block:
```diff new_file.txt -2 +2
+To my code
```
👏Comments are separated by ---.
I write those comments to a file, tweak them if necessary, and create a pending GitHub review with this code. I keep that file of comments as a place to track what comments have been addressed and how, whether a requested change was made (and in what commit) or an answer was given for a question.
One final advantage of using NeoVim is how easy it is to define key bindings for all these actions, especially repeated actions using vim-repeat, making code review very fluid.
There are, however, some pitfalls. One is that it's easy to delete chunks that have comments. To avoid losing work, I added NeoVim functions that write any deleted or truncated code that have comments into a separate file, which I later use to create the Markdown comments. Another problem is that I don't see anyone else's comments while reviewing, so I occasionally make duplicate comments or miss explanations that aren't in the code. The most annoying pitfall, though, is not having the latest revision locally, causingy my comments to end up on the wrong lines of the current revision. That's part of the reason the script which creates the review only creates a pending review, so I can check it before submitting (though finding pending comments in GitHub's review UI is still a challenge).
I volunteered to review these large PRs because I'm always curious where my personal workflow hits a breaking point, and finding the friction gives me an opportunity to improve my tools in ways that benefit small tasks too. If you have further suggestions, or ideas on how to mitigate the pitfalls, email me!