Display all comments in-line on the full diff page

[juhoautio]

Could the comments be displayed also on the CHANGES page?

Already now comments are of course written on the full diff (CHANGES tab):

But currently they're only displayed separately (DISCUSSION tab):

[amit1rrr]

I chose to not show posted comments under the CHANGES tab because there's no way to show all/most comments on the diff. Let me explain why.

• You push a commit which changes notebook cells x, y, z
• Colleague provides some suggestions and you modify the cells (split, merge, delete etc)
• Now the original comments for cell x, y, z have no place in this new diff because x, y, z cells are not there anymore (they got split, merged, deleted remember?)

This cycle continues many time over in a typical PR lifecycle. The only comments we can show on the CHANGES tab are the latest posted comments and maybe some older comments if there were only intra-cell changes.

The same logic applies to GitHub PR diffs as well. E.g. I started 3 discussions on this PR today but diff can only show one.

Since we can't show most conversations on the diff anyway, I feel it's cleaner to show them all under the CONVERSATIONS tab. I hope that makes sense.

[juhoautio]

E.g. I started 3 discussions on this PR today but diff can only show one.

Yes, Github hides comments from the full diff if the content on that line has changed after the comment was made. I think it's still valuable even showing the one, in your PR's case.

All comments (even "outdated" ones) can still be found on the discussions tab.

In some cases the easiest way to comprehend what comments are about is seeing them together with the full context.

I still think that matching github's approach on this would be a beneficial feature for ReviewNB.

[amit1rrr]

If you think more context on the comment (cells before/after) would help then we have 2 options,

1. Match GitHub's approach and show the comments where we can. This has the risk of not having context on most comments.
2. For each comment thread in the conversation tab provide ability to expand the context E.g. Show 3 cells above this context cell. This provides context for all conversations but context might be limited, say upto 5 cells above and below or something like that.

Which approach would you find useful? Depending on our conversation I am going to create a feature request on this Repo and can talk to other users and get more inputs.

[juhoautio]

Ultimately it could be both. Outdated comments are more or less throw-away though. If it were up to me I would for sure focus on 1. rather than 2.

For outdated comments Github seems to preserve some ~5 lines before the comment. What would work for Notebooks? Maybe ~2 preceding & ~1 following cells (could depend on the size of cells).

Interested to see how at least 1. would fly as a feature request. To me it seems suprising that this hasn't been raised before, but maybe other users don't see it as any major shortcoming.

[juhoautio]

One more thing – currently the context (cell) that's shown in ReviewNB's DISCUSSION tab is the plain cell content. This is demonstrated by the screenshots in this issue's initial comment. It could be also the actual diff? Once again, that's what Github does in discussion (both outdated & non-outdated comments).

[amit1rrr]

For outdated comments Github seems to preserve some ~5 lines before the comment. What would work for Notebooks? Maybe ~2 preceding & ~1 following cells (could depend on the size of cells).

I actually think the cell itself is (or should be) enough context for the comment to stand upon but I might be wrong. I have created this as a feature request, let's see how it does: https://github.com/ReviewNB/support/issues/34

currently the context (cell) that's shown in ReviewNB's DISCUSSION tab is the plain cell content. It could be also the actual diff?

Yeah, it would be nice but just comes at cost of latency. It's because we do not store diffs (or any repository contents for that matter). We just say 7th cell in this notebook has comment X and that information (cell_index=7) is also stored in your GitHub comment itself (as a non-visible div tag). So to actually highlight the cell we'll have to compute the diff again at every commit point where there is even one comment. With large number of files/commits/comments in the PR it just gets out of hand. We parallelize a lot but are still bound by GitHub API limits and such.

Maybe we could highlight the context cell diff when number of diffs to compute are manageable (depending on number of files/commit/comments). But I'm not sure if that's a good user experience, user be like "these cells were being highlighted until yesterday, now it's all plain what does that mean". You know what I mean :) What are your thoughts?

[juhoautio]

I actually think the cell itself is (or should be) enough context for the comment

I agree.

It's because we do not store diffs Maybe we could highlight the context cell diff when number of diffs to compute are manageable

I see. If it's not technically feasible to do it properly, it's not worth it.

To conclude this discussion, I like what you posted as https://github.com/ReviewNB/support/issues/34. Thanks!