Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • Oct 27 18:42
    gsuess edited #782
  • Oct 27 18:42
    gsuess edited #782
  • Oct 27 18:38
    gsuess opened #782
  • Oct 27 04:51
    pkaminski labeled #781
  • Oct 27 04:51
    pkaminski opened #781
  • Oct 25 21:02
    pkaminski closed #692
  • Oct 25 21:02
    pkaminski commented #692
  • Oct 25 20:51
    pkaminski labeled #734
  • Oct 25 20:48
    pkaminski labeled #753
  • Oct 24 18:57
    duarten closed #780
  • Oct 24 06:07
    pkaminski commented #780
  • Oct 24 05:29
    duarten commented #780
  • Oct 24 05:05
    pkaminski commented #780
  • Oct 24 04:56
    duarten opened #780
  • Oct 20 05:38
    Client build 4290 deployed
  • Oct 18 21:05
    Client build 4288 deployed
  • Oct 18 17:21
    pkaminski closed #757
  • Oct 18 17:21
    pkaminski commented #757
  • Oct 18 09:07
    badz0 commented #757
  • Oct 17 23:27
    pkaminski commented #285
Piotr Kaminski
@pkaminski
OK, I deployed an instrumented version. Please try loading the page again with ?debug=bounds appended (but before any anchor) and let me know what gets spewed to the console. Given how many files are in that review I'd expect a few hundred lines.
Misha Gusarov
@dottedmag
@pkaminski https://gist.github.com/dottedmag/4833abc4372baac53308b5b4e28014e1 — last 6 lines are repeating forever
Piotr Kaminski
@pkaminski
Thanks. This is definitely the issue, now I just need to interpret what I'm seeing. :)
Piotr Kaminski
@pkaminski
Arrgh. lib/monitoring/context.go was renamed at r1 so it doesn't "exist" at that revision, but Reviewable is trying to pick that as the right diff bound anyway. Probably because it's a "mixed" file: its r1 was assimilated into a new renamed file, but it gets re-added at r3, so it's not completely gone.
Piotr Kaminski
@pkaminski
@dottedmag OK, I released a candidate one-liner fix, please give it another try!
Misha Gusarov
@dottedmag
You've got a crash report, I believe.
TypeError: Cannot read property 'assimilated' of undefined
Persists over refreshes.
Piotr Kaminski
@pkaminski
Yep, pushing fix right now.
Live, try again?
Misha Gusarov
@dottedmag
Works! Thanks.
Piotr Kaminski
@pkaminski
Woot! Thanks so much for helping me figure out this corner case. :)
Misha Gusarov
@dottedmag
I suppose amount of various permutations of file states is mind-boggling.
Piotr Kaminski
@pkaminski
Partially-assimilated files are the trickiest kind of renamed file (which are already plenty tricky!) but they're reasonably well tested. What caused the issue in this case was having the review style be by-commit at the same time. Yay for unexpected interactions...
Dan Halperin
@dhalperi

Dumb q: is it just me or did the highlighting of text change in the last few months?

I think that when I made a multiline selection in the past, reviewable would automatically expand the front of the selection to the entire first line, and maybe do the same for the end. Now it seems to be keeping my first line exactly where it was.

Is this an intended change? If so, fine - I can be more precise :)

(I'm talking about multiline selection -> quote in comment, specifically)
Piotr Kaminski
@pkaminski
Sounds like a regression. I fixed a bug (for Firefox, IIRC) where the selection was being over-extended, so I wouldn't be surprised if it was under-extending somewhere now. :(
Dan Halperin
@dhalperi
K. Would a screencap or other info be useful? (As always, I use Chrome latest on MacOS latest, with animations disabled)
Or I can file an issue.
Piotr Kaminski
@pkaminski
Actually, looks like it's just completely broken. Arrgh. I'll get it fixed tonight...
I was getting some weird errors about event.target.closest being undefined that I kludged away, but perhaps broke things doing that...
Piotr Kaminski
@pkaminski
Turns out it was a regression from the last phase of migrating the review page to Vue, that I failed to catch during code review because Reviewable can't yet manage cross-file block moves. :( Should be fixed now.
Dan Halperin
@dhalperi
Seems to be working, thank you. Fast as always!
Alexey Feldgendler
@feldgendler
Hi! In the My Reviews dashboard, where do the unreviewed files counts come from?
I often see small values like 1 there, but when I click a review, it has dozens of unreviewed files.
Piotr Kaminski
@pkaminski
Those are recalculated on the fly, but without all the information available on the actual review page. In practice, they should only differ when the PR has renamed files, and if they do differ they should show "N+" to indicate that there might actually be more. Are you observing something different?
Alexey Feldgendler
@feldgendler
@pkaminski Yes, I often see numbers (without a plus sign) that are smaller than the actual number of unreviewed files.
Piotr Kaminski
@pkaminski
Can you please point to a sample PR? (Ideally not under active review and with not too many files, as it'll make things easier to debug.)
Alexey Feldgendler
@feldgendler
@pkaminski Today I even saw a review that appeared in My Reviews without an unreviewed number at all, and it did have unreviewed files inside.
@pkaminski I can hold a review for you (not touch for a while). Can you ping me when you have the time to investigate? I'll find one and tell everyone not to touch it.
Piotr Kaminski
@pkaminski
@feldgendler If you're around, I have a few hours available starting now-ish.
Piotr Kaminski
@pkaminski
Given our offset timezones, perhaps it would make sense for you to prep a PR at the end of your workday so I could investigate in my morning time.
Marc Zych
@marczych

Are there any settings for not requiring review of files that only have base commit changes? We're getting some review fatigue from developers e.g. open PR, reviewer reviews all files, author merges the target branch, and the author asks the reviewer on slack to rubber stamp the slew of files that only have base commit changes.

I'm seeing baseCommitSha in the completion condition input but I suspect that it can't be used to detect when a file only has base commit changes since it was last reviewed. Any ideas?

4 replies
Alexey Feldgendler
@feldgendler
@pkaminski Alright, try this one: https://reviewable.io/reviews/ridge/tectonic/7685
Piotr Kaminski
@pkaminski
Thanks, could you please add screenshots of what you see on the dashboard and the review page for this PR?
Alexey Feldgendler
@feldgendler
It appears for me in “My Reviews” (“Created by me” section) with the number 3 in the unreviewed files column, but the PR actually has 8 unreviewed files.
Piotr Kaminski
@pkaminski
Just "3" not "3+", right?
Also, can you tell from eyeballing the review which number is accurate (if any)?
Alexey Feldgendler
@feldgendler
@pkaminski Yes, “3” on grey background.
The correct number is 8.
Piotr Kaminski
@pkaminski
OK, I see no review marks for any files/revisions, so that makes sense.
Could you please load the dashboard page with ?debug=model appended and report the value of truss.store.reviews['-MKLW60F3jteR--jiz2L'].files.num?
Piotr Kaminski
@pkaminski
(Particularly interested in estimate, reviewable, unreviewed, and toReview on that object.)
Piotr Kaminski
@pkaminski
@feldgendler ^^
Piotr Kaminski
@pkaminski
So it's suggestive that there are 3 changed files at r2 and all others carried over but I'll need the numbers above to help me trace the causality chain.
Alexey Feldgendler
@feldgendler
@pkaminski Of course, that PR is no longer in this state. I'll try to catch one more and will include the debug info.
Piotr Kaminski
@pkaminski
Note that the review will have a different ID that I'll need to look up for you.
Marc Zych
@marczych
Hi again. I have a PR that has commits that aren't shown in Reviewable despite multiple pushes to try to resync it. Is there a way to force resync a PR?
Also, a different PR didn't get the Reviewable button added to the PR description until I manually navigated to the review. Even then, the first few times displayed "Unable to create review: Request queued but server did not respond" but eventually it worked. I wonder if we're getting rate limited or there is some other issue with our GHE instance. Any ideas?
Piotr Kaminski
@pkaminski
@marczych It sounds like your Reviewable servers might be getting overwhelmed. Probably worth looking at queue health indicators in the logs, and poking around /queues/githubPullRequestSync in the Firebase console to see if any tasks have been attempted a large number of times. (For the review that just won't resync, it's also possible that you've run into a fatal crash driven by the data specific to that PR, so the task can't complete. You should look for "Repeatedly failed to process event" in the logs.)
Marc Zych
@marczych
Great, thank you for the pointers!