Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comments tied to a Standalone notebook are invisible and easily lost #97

Closed
tobiszuts-at-affirm opened this issue Jul 15, 2021 · 4 comments

Comments

@tobiszuts-at-affirm
Copy link

tobiszuts-at-affirm commented Jul 15, 2021

Problem
It's possible to add comments to a notebook that's viewed in 'Standalone' mode. However, doing so has two bugs:

  1. the owner of the PR is not automatically notified of these comments (they would need to go to the same notebook in Standalone mode and toggle the switch to subscribe to comments).
  2. New commits cause the comments (which are tied to the old standalone notebook) to be lost, since old commits no longer have associated Standalone notebooks: this is the far more serious problem.

Potential solution
The ideal behavior, from my perspective, would be to log those comments in the main discussion page of the PR (just like when commenting in a notebook not in Standalone mode), send out emails, and retain them even with new commits.

@tobiszuts-at-affirm tobiszuts-at-affirm changed the title Comments tied to a stand-alone notebook are invisible and easily lost Comments tied to a Standalone notebook are invisible and easily lost Jul 15, 2021
@amit1rrr
Copy link
Member

amit1rrr commented Jul 16, 2021

@tobiszuts-at-affirm Thanks for reporting.

There are 2 types of comments in ReviewNB-

  • PR Comments: These are posted on a pull request diff.
  • JDoc Comments: These are posted on a standalone notebook file.

These are two separate functionalities built for slightly different use cases that you can read more about here. The crux of the issue is, the standalone notebook (and any comments on it) are not tied to any pull request as such. Standalone notebooks are more like a GoogleDoc for Jupyter Notebooks.

Some of the confusion might be because we link to standalone notebook from the PR diff page. So it feels like they're connected to the PR (but they're not).

the owner of the PR is not automatically notified of these comments (they would need to go to the same notebook in Standalone mode and toggle the switch to subscribe to comments).

Everybody "watching" the repository on GitHub are automatically notified of comments on any standalone notebook in the repo. So anyone who's interested in receiving updates should just mark themselves as "watching" the repository on GitHub. As I explained above, standalone notebooks are not tied to any PR so we don't automatically subscribe the PR author to these notifications.

New commits cause the comments (which are tied to the old standalone notebook) to be lost, since old commits no longer have associated Standalone notebooks

All comments on a standalone notebook should be visible on the same standalone notebook (even if the comments are made at different commits). The only way you could loose comments is if subsequent commit renamed the notebook. If this is happening outside of that, do let us know & we'll look into it.

@tobiszuts-at-affirm
Copy link
Author

tobiszuts-at-affirm commented Jul 16, 2021 via email

@amit1rrr
Copy link
Member

amit1rrr commented Jul 21, 2021

Still, it would be a nice feature not to lose any comments when moving a notebook around:

I agree 100%. I've opened an issue to track this.

such a simple action can cause comments to be deleted forever.

These comments are not really deleted, they're still available, just that they're tied to OLD file name + path. E.g. if you move the file to it's old path+name you will see all the comments again. Of course, this is not ideal at all. I'm just suggesting you a workaround for now to recover any important conversations that you might have lost as part of moving files around.

If the above workaround is not good enough, feel free to write to me at [email protected] with old & new file paths & we would help you with manual migration of conversations.

@tobiszuts-at-affirm
Copy link
Author

tobiszuts-at-affirm commented Jul 21, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants