Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • Dec 04 00:54
    Server build 3664 deployed
  • Dec 01 23:58
    pkaminski commented #981
  • Dec 01 22:04
    pkaminski commented #981
  • Dec 01 22:02
    pkaminski assigned #981
  • Dec 01 22:02
    pkaminski labeled #981
  • Dec 01 21:35
    jlinder opened #981
  • Nov 29 19:15
    Client build 5716 deployed
  • Nov 23 05:54
    Client build 5715 deployed
  • Nov 23 04:33
    Client build 5714 deployed
  • Nov 23 04:23
    pkaminski closed #514
  • Nov 22 05:13
    Server build 3663 deployed
  • Nov 22 05:05
    Client build 5707 deployed
  • Nov 17 13:41
    EricCousineau-TRI commented #17
  • Nov 17 13:41
    EricCousineau-TRI commented #17
  • Nov 17 13:38
    EricCousineau-TRI commented #222
  • Nov 17 13:36
    EricCousineau-TRI commented #17
  • Nov 17 13:36
    EricCousineau-TRI commented #17
  • Nov 17 13:34
    EricCousineau-TRI commented #17
  • Nov 17 13:33
    EricCousineau-TRI commented #17
  • Nov 16 04:50
    Client build 5703 deployed
Stephen Holsapple
@sholsapp

Hi team, first post here.

I’m trying to enable more and more Github’s branch protections so we have enforcement of some of our policies around code review. I’m having trouble with the Github branch protection described as, “Require conversation resolution before merging”.

For folks using Github's review system, this works fine. However, for folks using Reivewable's review system, it doesn't appear that discussions/comment disposition makes its way into Github's system of record (what I'm calling the place where Github squirrels away all the data under the hood like review status, open discussions).

I think I see what is going on: Reviewable is sync'ing discussions sourced from Reviewable's UI into Github as general comments, as opposed to review comments. The distinction is a general comment does not need acknowledge/resolution from anywhere.

In Github's UI, you create these general comments by navigating to the bottom of a PR and punching in some text to input form there, and push the green "comment" button. In Github's UI, you create a review comment by first navigating to "Start a Review", and then leaving a comment this way.

Has anyone else experienced this and has a solution that would cause Reviewable sourced discussions to show up as Github review comments instead of general comments? I think if you could do this, Github's branch protections would work fine.

2 replies
Dan Halperin
@dhalperi

Reviewable seems like it's taking a LOOOONG time to load a PR. The network seems fine. Is there any good flags to set to debug?

Wondering if it's related to codecov integration that we recently turned on. Is loading that integration blocking the diff loading?

21 replies
Dan Halperin
@dhalperi
Screen Shot 2022-04-25 at 4.13.15 PM.png
Dan Halperin
@dhalperi
Screen Shot 2022-04-25 at 4.23.18 PM.png
Michał Kowalczyk
@mkow
feature idea: what do you think about changing how the "Mark reviewed and go to next file" button works? the problem I have with it is that it goes to the next file with some changes, but jumps over discussions with new comments
optimally I'd like it to jump to the next item which has new contents since the last review
if you like the idea I can file an issue for further discussion ;)
Piotr Kaminski
@pkaminski
So basically a combination of x and .? Dunno, not sure that would fit more people's workflows than the current approach. You can fake it with a compound keyboard shortcut if you want, though, just bind to toggleCurrentFileReviewed(true); nextChangedFileOrUnrepliedDiscussion().
Michał Kowalczyk
@mkow
the problem with using this button is that you sometimes need to manually go back to reply to some discussions
and it's IMO unintuitive that it actually skips some of the updates
so, as the effect, I often resort back to just page up/down
Piotr Kaminski
@pkaminski
Personally, I think pgup/pgdn is a more natural UI anyway, since we do our best not to show anything irrelevant on the page. The "mark and next file" button is more for when you get stuck in single-file mode, in which case skipping over a file isn't as big a deal and you just do a sweep for any remaining discussions at the end.
I dunno, I think it we went all-in on the "next item" style navigation it would make sense, but given that right now we emphasize cycling files vs discussions separately it's reasonable to keep this button file-specific. That said, should you open an issue it would allow other people to chime in and give me an opportunity to change my mind. :)
Michał Kowalczyk
@mkow
tbh I'm not against the pgup/pgdn navigation in principle, but the problem with it is that it doesn't focus on anything, so I need more mouse moving and clicking to do anything
anyways, this isn't a big issue for me
just a small cumbersomeness, i.e. no perfect way to read everything I want without scrolling through things which didn't change ;)
Michał Kowalczyk
@mkow
@pkaminski can I quote you in the issue? especially that "I think it we went all-in on the "next item" style navigation it would make sense, but given that right now we emphasize cycling files vs discussions separately it's reasonable to keep this button file-specific.", which sounds like a good argument against this change
I'll create the issue as I'm curious what other people will say :)
Piotr Kaminski
@pkaminski
Of course -- thanks!
Michał Kowalczyk
@mkow
I actually wonder how many different flows do people have to go through the reviews
Arran Schlosberg
@arran-fathom
I'm so glad I found Reviewable, thank you! The custom syntax highlighting is giving me some trouble though, is it ok to just use a GitHub raw content link https://raw.githubusercontent.com/highlightjs/highlight.js/main/src/styles/monokai.css?
Piotr Kaminski
@pkaminski
Thanks @arran-fathom! You can't use a raw content link directly because it doesn't serve with the correct content type, but all you need to do is go to https://raw.githack.com/ and paste in the raw link to get a usable one.
Arran Schlosberg
@arran-fathom
That @pkaminski :)
Arran Schlosberg
@arran-fathom
*Thanks
Misha Gusarov
@misha-ridge
@pkaminski Hello Piotr. ridge/tectonic#15318 says: "code-review/reviewable Expected — Waiting for status to be reported ". However the review there was finished 15:01 UTC
So Reviewable shows the big green button "ready to merge", but our bot that watches the checks status does not want to merge the PR.
Misha Gusarov
@misha-ridge
@pkaminski Interesting. This PR has got a conflict, so there is no "merge" button in GH interface, but Reviewable proposes to merge it.
Piotr Kaminski
@pkaminski
Strange, let me look it up.
Here's the proximate culprit, now to dig to root cause: "Dropping mergeability due to last revision commit mismatch for ridge/tectonic#15318"
Piotr Kaminski
@pkaminski
It looks like Reviewable is refusing to match GitHub's commit sequence for this PR. GitHub's ends in 5e1a516 then c45a3e1, while Reviewable has those but then goes back to 5e1a516 as the last commit for some reason.
Misha Gusarov
@misha-ridge
Maybe a clue: the list of commits is not linear. 464d46e is a commit on a branch, then 04e2943 is a merge of 464d46e and master, then e3a1ea0 is a commit on top of 464d4e, then 5e1a516 is a merge of e3a1ea0 and master, and finally c45a3e1 is a merge of 04e2943 and 5e1a516
I know it's a messy branch. Sorry about that :)
Piotr Kaminski
@pkaminski
We sort the commits topologically so I don't understand how 5e1a516 could've ended up after c45a3e1 in Reviewable's ordering. Was there ever a moment when 5e1a516 was the head of the branch after c45a3e1 had already been created?
At this point, Reviewable is stuck because it sees that all commits have been captured, yet the last revision isn't at the current head. However we got there this is a situation we need to be able to recover from -- let me think about it a bit after my upcoming meeting, and I'll endeavor to push out a fix later today.
Misha Gusarov
@misha-ridge
No, these two commits were pushed at the same time.
Thanks
Piotr Kaminski
@pkaminski
Hey @misha-ridge, a proper fix will take some thinking as it gets to an underlying design question we've been struggling with for years. In the meantime, I manually adjusted the review so that r3 is considered obsolete and r2 is the latest revision -- I believe that should get things unstuck.
Piotr Kaminski
@pkaminski
I also still don't understand how the review got into this state. Both r2 and r3 were created in one push, with both 5e1a516 and c45a3e1 already on the branch, so c45a3e1 should have ended up as the last revision no matter what unless the topological sort when haywire.
Piotr Kaminski
@pkaminski
(Actually, I had to delete r3 altogether, otherwise it would just get un-obsoleted on sync. Let me know if this helped or not!)
Misha Gusarov
@misha-ridge
@pkaminski Thanks. The PR is now in a sane state.
Dan Halperin
@dhalperi
Is there a way to get a github link from an arbitrary file? I know reviewable does this for large files, and maybe images, but I'd like it for an arbitrary file. In this case (https://reviewable.io/reviews/batfish/pybatfish/837/jupyter_notebooks/Pandas%20Examples.ipynb#-) I want to review GitHub's Jupyter notebook preview.
Piotr Kaminski
@pkaminski
We don't surface it at the file level yet, but if you create a draft then there's a GitHub link behind the "line N" label in the upper-right corner.
Dan Halperin
@dhalperi
:+1:
Artiom Levinton
@artiomle

Hi Piotr, I have some sttange problem in our Reviewable container, it crushed with the following error message

'Unable to initialize generic GitHub access via "github:24". Unknown encryption prefix: undefined'

I have tried to recreate the app secret token and re authorize my session with github (im user 24)
Piotr Kaminski
@pkaminski
(Moving conversation to private channel.)
Michał Kowalczyk
@mkow
just want to say: I really like this new "pondering" state of discussion, it was a nice idea :)
Piotr Kaminski
@pkaminski
Thanks, that's great to hear! All credit goes to @jwnimmer-tri, though. :)
Misha Gusarov
@misha-ridge

Hey. Is there a way to "see myself out" out of a discussion thread in Reviewable? I commented on a thread in a long-running PR, and every day there is a new comment there, and it pops up in my "things to look at", even though I really don't want to look at it anymore.

I know it's a bad idea to have long-running PRs, but we have them from time to time.