Fix loading content history on show more

[Gusted]

• Call initRepoIssueContentHistory so that the newly loaded comments also get their content history.
• Resolves Code comment history not shown on lazily loaded diffs #17767

[silverwind]

Kind of unrelated but I don't like it that initRepoIssueContentHistory does an unconditional fetch request. I feel like this is something that could have been rendered directly into the HTML instead.

It might be better to do that fetch only when clicking the edit history, via a delegated event listener on a parent node.

[wxiaoguang]

I think we should prevent init something again and again. init usually means a one-time work.

As silverwind suggested, we may try to attach some event listeners to parent DOM node. ps: "something have been rendered directly into the HTML instead" seems more complex than this solution, it would break related code into many functions.

Or, we can use a re-call safe function, eg: attachXxx, like the existing attachTribute, then developers are clear that it can be called on demand. And in the attachXxx, we can only fetch the new content history data we need.

[silverwind]

I guess it's a good short-term fix.

[jpraet]

Note that the ticket #17767 also mentions the scenario where a diff of a file is initially hidden because it is too large.
When you click "Load Diff", the comment edit history is missing too. I think this PR currently only handles the "Show More" scenario.

[zeripath]

So we'd need to add:

diff --git a/web_src/js/features/repo-diff.js b/web_src/js/features/repo-diff.js
index 5f231b6c8..f8f6c38b7 100644
--- a/web_src/js/features/repo-diff.js
+++ b/web_src/js/features/repo-diff.js
@@ -127,6 +127,7 @@ export function initRepoDiffShowMore() {
       }
 
       $target.parent().replaceWith($(resp).find('#diff-file-boxes .diff-file-body .file-body').children());
+      initRepoIssueContentHistory();
     }).fail(() => {
       $target.removeClass('disabled');
     });

if we wanted to catch the show diff too.

I think however, we should consider if we want to load the history like this - would it be better to have it be dynamically loaded when the user requests to see the history?

[Gusted]

I think however, we should consider if we want to load the history like this - would it be better to have it be dynamically loaded when the user requests to see the history?

I think it's best if we add a attribute to a comment if it has history, and the front-end would request the history on demand. Any objections?

[zeripath]

No objections from me. This would simplify a lot of things and reduce the risk of serious problems with heavily edited issues and PRs.

[wxiaoguang]

I think however, we should consider if we want to load the history like this - would it be better to have it be dynamically loaded when the user requests to see the history?

I think it's best if we add a attribute to a comment if it has history, and the front-end would request the history on demand. Any objections?

I think it can be done mostly in frontend, then we do not need to change backend code (no more dependencies or coupling, no need to make template files more complex)

1. initRepoIssueContentHistory() => attachRepoIssueContentHistory(targetCommentId=null)
2. GET /content-history/overview => POST /content-history/overview with {commentIdList: [...] }, only query the overview information for commentIdList
3. If targetCommentId==null, attachRepoIssueContentHistory would collect all history-menu-not-attached comment IDs, and send to backend to query overview.
4. Then re-calling attachRepoIssueContentHistory(null) won't send redundant requests, we can call it safely at any time (Improvement)
5. If a content is edited, we can call attachRepoIssueContentHistory(commentId) to do a refresh and show the history menu (Improvement)