feat: add octo review browse to browse a pull request #830

ldelossa · 2025-01-29T21:24:34Z

Add a new command Octo review browse which opens the review layout for a PR but does not start a review.

This is handy when you want to look over the changes in a PR but do not necessary want to review it.

When you are in a Review layout initiated by Octo review browse you can intuitively convert it into an active review session with Octo review start.

Its invalid to call Octo review browse after Octo review start and the user must either submit or discard the active review to enter the browse state again.

Closes: #829

Describe what this PR does / why we need it

Provides the ability to browse a PR but not enter a review

A big reason to do this is say, you already submitted a review, but you want to use the (very nice) Review layout to continue looking at other contributor's comments.

Does this pull request fix one issue?

#829

Describe how you did it

Copied the patterns used for Octo review start but instead of submitting graphql mutations to start a review, we simply just get the review threads and initialize the review layout. The abstractions are well done, so this was clean and done without much trouble.

Describe how to verify it

Open a PR with Octo pr edit|list and issue Octo review browse you can then ensure starting a review from the browse state is working with Octo review start.

Special notes for reviews

Checklist

Passing tests and linting standards
Documentation updates in README.md and doc/octo.txt

Add a new command `Octo review browse` which opens the review layout for a PR but does not start a review. This is handy when you want to look over the changes in a PR but do not necessary want to review it. When you are in a Review layout initiated by `Octo review browse` you can intuitively convert it into an active review session with `Octo review start`. Its invalid to call `Octo review browse` after `Octo review start` and the user must either submit or discard the active review to enter the browse state again. Closes: pwntester#829 Signed-off-by: ldelossa <[email protected]>

ldelossa · 2025-01-30T00:08:45Z

going to convert this to draft.

needs docs update and I want to handle this:

A bit better.

Any early comments are appreciated however.

phdah · 2025-01-30T12:59:53Z

Tested the draft version out. It's great, but agree about the commenting issue. Expected behavior would be for it to be just a non-review comment, or less expected but that it triggers first Octo review start if comment/suggestion is invoked.

wd60622 · 2025-01-30T13:04:35Z

Very cool. What is the behavior when you (try to) add a comment while browsing? Does that automatically start a review? Or cause an error?

Also, can Octo review start be called while browsing?

wd60622 · 2025-01-30T13:06:22Z

lua/octo/reviews/init.lua

+  gh.run {
+    args = { "api", "graphql", "-f", string.format("query=%s", query) },
+    cb = function(output, stderr)
+      if stderr and not utils.is_blank(stderr) then
+        utils.error(stderr)
+      elseif output then
+        local resp = vim.fn.json_decode(output)
+        callback(resp)
+      end
+    end,
+  }


Interested in using the new pattern?

gh.api.graphql { query = query, opts = { cb = cb, }, }

Definitely, that is very nice. Will update.

wd60622 · 2025-01-30T13:08:20Z

lua/octo/reviews/init.lua

+  local file = io.open("/tmp/query", "w")
+  file:write(query)
+  file:close()


Is this just for testing?

Yup 🤦

forgot about that. Ill remove.

ldelossa · 2025-01-30T13:32:24Z

@phdah @wd60622

as far as creating a comment via in "browse" state.

For now, I think it will just create a notification asking you to start a review. Or, if we want, start a review automatically, but youd need to do a "Octo comment add" once again, since starting a review reloads the layout, and puts you at the first changed file. This maybe not where you wanted to comment.

As a follow up, id like this to add a non-review commit! Theres some nuance to this however, since it, iirc from when I worked on this, requires the use of a separate set of APIs. So we need to work in some logic to say "hey is this a non-review commit, if so, use this api and not the review comments api".

Id like this, but probably in a follow up pr? Thoughts?

Re:

Also, can Octo review start be called while browsing?

yup, that use case is implemented. From browse you can go right to start. From within a review, you cant go to browse. Youll need to submit or discard the current review. This makes things a bit simpler.

GuillaumeLagrange · 2025-01-30T21:55:04Z

I like the idea of not having to start a review in order to post a comment or browse diffs, very much so!

I can't think of a good UX in the current octo way of doing things to seemlessly allow users to start reviews while creating comment, i'd like to avoid being prompted everytime we add a comment.

The best idea I have right now is that when a user runs Octo command add

If no review is in progress, add the comment in a standalone way, just like when one clicks here
If a review is in progress, add the comment to the review

And let the user start a review if they wish to do so using Octo review

WDYT ?

While we're touching this part of the code, do you think this can address #823 ?

ldelossa · 2025-01-31T16:43:58Z

@GuillaumeLagrange

I totally agree.

Ultimately I think 'Octo command add' in the 'browse' state should add a non-review commit.
I was originally thinking of implementing this in a second PR since I need to dig into the API changes required for this.

I was thinking of this PR just providing the initial implementation of the feature, and then following up with API changes to handle "browse state comments".

Would you rather see it all done in one PR here? I'm not too opposed, would just take a bit longer to get the Octo pr browse feature out to users.

GuillaumeLagrange · 2025-01-31T21:26:28Z

I am fine with iterating, as long as we do not hurt existing UX, which is not the case here.
Feel free to submit the PR without the comment management first!

Btw it's still in draft but let me know if you want a review

ldelossa · 2025-02-01T16:10:42Z

@GuillaumeLagrange sounds good. Yeah if we do not mind, ill probably submit this one without "non-review" comments, and then start working on making that work in a immediate subsequent PR.

phdah · 2025-02-14T07:14:40Z

@ldelossa, would you like to convert the PR from draft to ready? I've been using this for two weeks, and really appreciate the feature. Pushing the non-review sounds good.

ldelossa · 2025-02-14T13:57:10Z

@phdah I want to handle some error conditions when trying to apply comments when in browse mode. I'm a bit busy with day job right now, but will follow up on this next week.

ldelossa force-pushed the ldelossa/octo-review-browse branch from 18cdd1a to 6c2f501 Compare January 29, 2025 21:25

wd60622 requested a review from GuillaumeLagrange January 29, 2025 21:27

ldelossa marked this pull request as draft January 30, 2025 00:08

wd60622 reviewed Jan 30, 2025

View reviewed changes

wd60622 added enhancement New feature or request command Dealing with Octo commands labels Jan 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add octo review browse to browse a pull request #830

feat: add octo review browse to browse a pull request #830

ldelossa commented Jan 29, 2025 •

edited

Loading

ldelossa commented Jan 30, 2025 •

edited

Loading

phdah commented Jan 30, 2025 •

edited

Loading

wd60622 commented Jan 30, 2025 •

edited

Loading

wd60622 Jan 30, 2025 •

edited

Loading

ldelossa Jan 30, 2025

wd60622 Jan 30, 2025

ldelossa Jan 30, 2025

ldelossa commented Jan 30, 2025 •

edited

Loading

GuillaumeLagrange commented Jan 30, 2025 •

edited

Loading

ldelossa commented Jan 31, 2025

GuillaumeLagrange commented Jan 31, 2025 •

edited

Loading

ldelossa commented Feb 1, 2025

phdah commented Feb 14, 2025

ldelossa commented Feb 14, 2025

feat: add octo review browse to browse a pull request #830

Are you sure you want to change the base?

feat: add octo review browse to browse a pull request #830

Conversation

ldelossa commented Jan 29, 2025 • edited Loading

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

Checklist

ldelossa commented Jan 30, 2025 • edited Loading

phdah commented Jan 30, 2025 • edited Loading

wd60622 commented Jan 30, 2025 • edited Loading

wd60622 Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

ldelossa Jan 30, 2025

Choose a reason for hiding this comment

wd60622 Jan 30, 2025

Choose a reason for hiding this comment

ldelossa Jan 30, 2025

Choose a reason for hiding this comment

ldelossa commented Jan 30, 2025 • edited Loading

GuillaumeLagrange commented Jan 30, 2025 • edited Loading

ldelossa commented Jan 31, 2025

GuillaumeLagrange commented Jan 31, 2025 • edited Loading

ldelossa commented Feb 1, 2025

phdah commented Feb 14, 2025

ldelossa commented Feb 14, 2025

ldelossa commented Jan 29, 2025 •

edited

Loading

ldelossa commented Jan 30, 2025 •

edited

Loading

phdah commented Jan 30, 2025 •

edited

Loading

wd60622 commented Jan 30, 2025 •

edited

Loading

wd60622 Jan 30, 2025 •

edited

Loading

ldelossa commented Jan 30, 2025 •

edited

Loading

GuillaumeLagrange commented Jan 30, 2025 •

edited

Loading

GuillaumeLagrange commented Jan 31, 2025 •

edited

Loading