Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • Mar 06 12:09
    Server build 2886 deployed
  • Mar 06 10:17
    bstaletic opened #821
  • Mar 06 08:19
    Server build 2884 deployed
  • Mar 06 03:35
    pkaminski opened #820
  • Mar 06 03:35
    pkaminski labeled #820
  • Mar 05 15:33
    pkaminski labeled #819
  • Mar 05 15:33
    pkaminski opened #819
  • Mar 04 14:58
    Server build 2857 deployed
  • Mar 04 07:44
    Server build 2854 deployed
  • Mar 04 07:11
    Client build 4638 deployed
  • Mar 02 16:44
    cgiroux86 assigned #804
  • Mar 02 15:54
    Client build 4633 deployed
  • Mar 02 04:18
    pkaminski commented #791
  • Mar 02 04:18
    pkaminski labeled #791
  • Mar 02 04:18
    pkaminski labeled #791
  • Mar 02 04:18
    pkaminski unlabeled #791
  • Mar 01 20:44
    jwnimmer-tri commented #791
  • Mar 01 15:58
    Server build 2827 deployed
  • Feb 26 07:05
    pkaminski closed #818
  • Feb 26 07:05
    pkaminski commented #818
Piotr Kaminski
@pkaminski
No trouble canceling a sub. You don't even need to provide a card, and you'll be able to complete any reviews in progress (but not start new ones). As for permissions, I'm not sure what GitHub's UI du jour is, but if you revoke authorization completely then Reviewable will start asking for it piece-by-piece again so you can get back to the desired state.
(Let me double-check that a subscription will let you turn off the badge in public repos, though -- that was my intent but it's a corner case that doesn't get exercised often.)
Shoot, the current code will always force the badge on in public repos. Is that an issue, or are you OK trying things out in a private repo?
xmo-odoo
@xmo-odoo
@pkaminski I can probably find PRs in the private repos to test things with, or I'll be careful to quickly go and edit the PR description to remove the button footer in the PRs I test reviewable on if they're public. Thanks for the help :)
Piotr Kaminski
@pkaminski
If you do the latter, keep in mind that Reviewable will do its level best to put the badge back in. :)
Qian Deng
@qian-deng
Sorry for the late response. We defined a required review workflow with Reviewable. An owner-file map was added to the completion condition script. For each changed file in a PR, we are able to find its owners if it has any and request review/approval from at least one of them. The owner-file map was built with OWNERS files in a repo. Since OWNERS file are changed regularly for some of our teams, we are looking for ways to update/sync Reviewable config more easily. We are able to detect changes in OWNERS files, update the owner-file map and inject it into the completion condition script automatically. As for the last step, we don’t know what would be the best option. We are considering Selenium but authentication might be a problem.
Piotr Kaminski
@pkaminski
@qian-deng Would injecting the current contents of the OWNERS file at the PR's base branch into your completion script solve your problem? I've been considering adding a feature to inject arbitrary file contents from the repo into the script for a while but was looking for a customer willing to work with me on it. (This would initially come with no UI and I'd just edit your desired list of files directly into the database. It would, however, automatically recompute conditions for open PRs when the base branch is updated -- including for changes to injected files.) What do you think?
Qian Deng
@qian-deng
That sounds good! I think you said reading files from a repo cannot be achieved in #722. How does injecting work in this case?
Piotr Kaminski
@pkaminski
It's less that it cannot be achieved and more that it gets tricky with corner cases such that I wasn't comfortable making this trade-off for a core feature where a reliable alternative was already working. In your case, though, there's not really a better alternative and I'd we do hit a corner case you'll be able (and motivated!) to remedy it promptly. I guess I'm just more comfortable doing this for an advanced feature, though I'm also slowly coming around to accepting the trade-offs for #722, especially since the migration to GitHub apps might never happen at this point.
Anyway, I'll see how quickly I can get something running. Please let me know which repo(s) and which files you'd like injected (here or in private) and I'll let you know once they're available to your script.
Qian Deng
@qian-deng
Cool! Let me bring back that idea to the team. I will send a support email for further details. :) One more thing, you dont need any source code access to the repo, right?
Michał Kowalczyk
@mkow
Hi all, I've noticed a problem recently: After I submitted a review, the PR author bugged me to reply to some comments which I supposedly left unanswered. I checked them and I'm quite sure I did wrote a reply. Normally I'd think that's just me misremembering it (or accidentally deleting it to rewrite and forgetting to write a new version), but this happened 2/3 times recently on other PRs. Anyone maybe had a similar thing?
ofc there's a possibility that I just shouldn't review code that late :) but looks a bit suspicious, as such thing never happened before to me
the comment generated on GitHub is also missing these comments
this time it happened on a pretty huge PR (~3k LoC delta, 72 files), previously I don't remember
Eric Linxie
@elinxie

On my work account, I am trying to rename a file and then modify it, but then when I do, in separate commits, I find that my original file is deleted. Then my co-workers complain because now they have to reread all the code and they can't see what's changed from the old file with the original name.

Github keeps track of these changes, but I was wondering if Reviewable would do anything about it. I don't want to make a new branch just to rename my file just so they could review it. And I don't want to make a new branch because every branch takes a while to merge due to the long time it takes to build and test.

Piotr Kaminski
@pkaminski
@mkow Thanks for the report -- I haven't heard anything similar recently (or ever?), though. I can't really think of a way where a draft gets deleted but the comment doesn't get posted either. (And 3k delta isn't particularly huge for Reviewable, BTW. :smile: ) For the generated GitHub message, though, there is some logic to check previously posted comment fingerprints to avoid posting duplicates in case the (non-atomic) process failed part way through on a previous publishing attempt. Were your suspicious publishing attempts "clean" (no errors) or did you have to work around any failures to get your comments out?
Piotr Kaminski
@pkaminski
@elinxie This is likely because Reviewable's rename detection algorithm works between revisions, rather than between commits like git's (and presumably GitHub's) does. Usually this is fine, but if you exceed a certain threshold of changes after renaming and the two commits get aggregated into a single revision then Reviewable can miss the transition. I agree it would be better if we could detect at least "clean" renames at the commit level, though this won't be easy to shoehorn into Reviewable's architecture. Could you please open a feature request to track? In the meantime, a workaround is to push your branch after the rename, load the review page, and force the revision to be snapshotted (e.g., by creating and deleting a draft comment). As long as the revision is showing up in normal type (not italicized) in the file matrix, it's snapshotted and will persist when you push further change commits later. This should allow Reviewable to detect the rename consistently. Sorry for the trouble!
Piotr Kaminski
@pkaminski
Never mind about opening an issue, I see that you already did. I'll follow up there!
Michał Kowalczyk
@mkow

Were your suspicious publishing attempts "clean" (no errors) or did you have to work around any failures to get your comments out?

unfortunately I don't remember, as I notice this problem a few days (and tens of other reviews) later

if no one else had such a problem then I'll just try to be more attentive when submitting reviews, maybe I'll be able to notice a pattern
Piotr Kaminski
@pkaminski
OK, let me know what you find. One thing to keep an eye out for is whether the "to reply" counter in the toolbar is still red -- if so, there are still discussions that need an answer (or at least Reviewable thinks so). If you counter is grey, you publish, and your coworkers still find replies missing, then there's a bug -- whether in the "to reply" counting code, or in the actual posting of the comments.
Michał Kowalczyk
@mkow
Good idea, I'll keep an eye on these numbers (but I already usually click through them and publish only after there are no red ones). Maybe I'll notice them increasing after publishing or some similar anomaly...
Alexey Feldgendler
@feldgendler
@pkaminski Hello! I've got several PRs where Reviewable all of a sudden shows the most recent commit as file deletion. The commit in question is a merge with conflict resolution, but it certainly doesn't delete any files.
Piotr Kaminski
@pkaminski
No recent changes in that area, AFAIK, so it shouldn't be a regression at least. If you let me know a sample PR or two (hopefully with not too many files) then I can look into it tomorrow -- it's getting very late over here (UTC+8).
Alexey Feldgendler
@feldgendler
@pkaminski https://reviewable.io/reviews/ridge/tectonic/9140 (but it's a private repository)
Deletion is misdetected at r10, and for further revisions diffs aren't shown at all. In reality, no files are delited.
Piotr Kaminski
@pkaminski
Very mysterious indeed. I can confirm that both files show up in the commit trees at r9 and r10, so it sure looks like an issue on Reviewable's side. I'll dig deeper later today.
Piotr Kaminski
@pkaminski
Ah, never mind: things are working as expected. The file wasn't deleted, but rather the merge put the current version of the file into the base branch. Reviewable thus marked the file as obsolete and stopped tracking it, since merging it will be a no-op. If the file changes again at head Reviewable will resume tracking and diffing it.
However, this shouldn't show up as a file deletion -- does Reviewable literally say somewhere that the file was deleted (red revision box, "File deleted at r10", etc.) or was that just your inference from the fact that revisions r10 and up were not tracked for the files?
Alexey Feldgendler
@feldgendler
@pkaminski Yes, when I try to diff between r9 and r10, it literally says “File deleted at selected revision”; r10 shows in red; further changes to these files (e.g. r11–r12) are not accessible.
Expected outcome: r10 should show up as a normal revision; I should be able to see the difference between r9 and r10, as well as beween r10 and r11.
Piotr Kaminski
@pkaminski
Thanks, I think the breakage was caused by having the entire PR reverted to base at r10, exercising a never-before-seen edge case in Reviewable. I pushed a fix going forward but it won't help PR 9140. I see that it's merged anyway so I assume that's fine.
Alexey Feldgendler
@feldgendler
@pkaminski Thanks a lot!
RodgerZhu
@RodgerZhu
Help Needed!
It will show OFFLINE when I open the link https://reviewable.io/
Could someone give me some suggestions?
Piotr Kaminski
@pkaminski
Hey @RodgerZhu, sorry to hear you're having trouble. What is the fine print reason at the bottom of the offline dialog? That will inform next steps to figure things out.
RodgerZhu
@RodgerZhu

@pkaminski thanks for your response. Below is the message shown at the bottom of the page:

O F F L I N E
We'll get you reconnected as soon as possible.
Check @reviewableio for outage announcements.

Reason: no connection to Firebase

Piotr Kaminski
@pkaminski
OK, looks like your browser is having trouble establishing a connection with the Firebase servers. I assume this was working before so let's try to figure out what changed. Did you install any new extensions recently? Was the local network recently reconfigured, perhaps with new firewall rules or a proxy? Does Reviewable work from another browser or over another network (e.g. your cellular connection)?
Another thing to try is to load Reviewable with debug=workers appended to the URL and see if any network errors appear in the console. (The query forces all connections to be made in the page's own process.)
If these don't produce results we can dig deeper into network debugging -- let me know!
Eran Sakal
@esakal
CleanShot 2021-03-03 at 20.35.21@2x.jpg

Hello! @pkaminski you will not remember me, I started using Reviewable more than two years ago and I love this product. I truly believe that it made our code better and the code review process much more pleasant so thank you for that. I even convinced the current project team to switch from Bitbucket to Github to be able to use Reviewable.

Now to my question. two years ago I wanted that multiple reviewers will be able to review the same PR and let each reviewer to manage the unread/read status of each file separately. So even if I reviewed 20 files, the second reviewer will see them as unread and will be able to update their status on its own. You guided me to update Condition function and provided me a text to copy-paste their to do that.

Can you please re-share the condition function to use to make each reviewer individual?

Piotr Kaminski
@pkaminski
Hey Eran -- good to hear from you again, and thanks for championing Reviewable! I don't recall exactly what code snippet I showed you two years ago, but the good news is that we'll be addressing this use case head-on pretty soon. Take a look at Reviewable/Reviewable#817, which also links to all the other related issues, many of which have code samples if you need to hack your way around the problem right now.
Eran Sakal
@esakal
It seems to solve it - Thanks!
Eran Sakal
@esakal
image.png
@pkaminski one more question. Is it possible to instruct Reviewable to ignore changes in whitespaces like those?
Piotr Kaminski
@pkaminski
Not ignore altogether, but you can hide them from diffs. The setting is pretty well hidden -- see towards the end of this section in the docs: https://docs.reviewable.io/files.html?highlight=whitespace#collapsed-sections
Boris Staletic
@bstaletic

Hello. I was certain that Reviewable can render markdown tables. The table is rendered properly in github, but reviewable is displaying "raw" markdown.

https://github.com/ycm-core/ycmd/pull/1540#issuecomment-790884779
https://reviewable.io/reviews/ycm-core/ycmd/1540#

Piotr Kaminski
@pkaminski
@bstaletic That's surprising, since Reviewable just uses GitHub's markdown rendering API. Could you please open an issue for this? Thanks!