fine tune

Author: wxiaoguang

routers/web/repo/compare.go

@@ -995,10 +995,15 @@ func ExcerptBlob(ctx *context.Context) {
 		if err != nil {
 			log.Error("GetIssueByIndex error: %v", err)
 		} else if issue.IsPull {
+			// FIXME: DIFF-CONVERSATION-DATA: the following data assignment is fragile
 			ctx.Data["Issue"] = issue
 			ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool {
 				return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee)
 			}
+			// and "diff/comment_form.tmpl" (reply comment) needs them
+			ctx.Data["PageIsPullFiles"] = true
+			ctx.Data["AfterCommitID"] = diffBlobExcerptData.AfterCommitID
+
 			allComments, err := issues_model.FetchCodeComments(ctx, issue, ctx.Doer, ctx.FormBool("show_outdated"))
 			if err != nil {
 				log.Error("FetchCodeComments error: %v", err)

services/gitdiff/gitdiff.go

@@ -191,7 +191,7 @@ func (d *DiffLine) RenderBlobExcerptButtons(fileNameHash string, data *DiffBlobE
 			link += fmt.Sprintf("&pull_issue_index=%d", data.PullIssueIndex)
 		}
 		return htmlutil.HTMLFormat(
-			`<button class="code-expander-button" hx-target="closest tr" hx-get="%s"  data-hidden-comment-ids="%s">%s</button>`,
+			`<button class="code-expander-button" hx-target="closest tr" hx-get="%s" data-hidden-comment-ids="%s">%s</button>`,
 			link, dataHiddenCommentIDs, svg.RenderHTML(svgName),
 		)
 	}

templates/repo/issue/view_content/conversation.tmpl

@@ -1,3 +1,8 @@
+{{/* FIXME: DIFF-CONVERSATION-DATA: in the future this template should be refactor to avoid called by {{... "." $}}
+At the moment, two kinds of request handler call this template:
+* ExcerptBlob -> blob_excerpt.tmpl -> this
+* Other compare and diff pages -> ... -> {section_unified.tmpl|section_split.tmpl} -> this)
+The variables in "ctx.Data" are different in each case, making this template fragile, hard to read and maintain. */}}
 {{if len .comments}}
 	{{$comment := index .comments 0}}
 	{{$invalid := $comment.Invalidated}}

web_src/css/review.css

@@ -127,7 +127,7 @@
   background-color: var(--color-primary);
   color: var(--color-primary-contrast);
   border-radius: 8px !important;
-  left: 4px;
+  left: -12px;
   top: 6px;
   text-align: center;
 }

web_src/js/features/repo-diff.ts

@@ -218,31 +218,40 @@ function initRepoDiffShowMore() {
   });
 }
 
-async function loadUntilFound() {
+async function onLocationHashChange() {
+  // try to scroll to the target element by the current hash
   const currentHash = window.location.hash;
-  if (!currentHash.startsWith('#diff-') && !currentHash.startsWith('#issuecomment-')) {
-    return;
-  }
+  if (!currentHash.startsWith('#diff-') && !currentHash.startsWith('#issuecomment-')) return;
+
+  // avoid reentrance when we are changing the hash to scroll and trigger ":target" selection
+  const attrAutoScrollRunning = 'data-auto-scroll-running';
+  if (document.body.hasAttribute(attrAutoScrollRunning)) return;
 
-  while (window.location.hash === currentHash) {
+  const targetElementId = currentHash.substring(1);
+  while (currentHash === window.location.hash) {
     // use getElementById to avoid querySelector throws an error when the hash is invalid
     // eslint-disable-next-line unicorn/prefer-query-selector
-    const targetElement = document.getElementById(currentHash.substring(1));
+    const targetElement = document.getElementById(targetElementId);
     if (targetElement) {
-      // re-trigger :target CSS and browser will scroll to the element
+      // need to change hash to re-trigger ":target" CSS selector, let's manually scroll to it
+      targetElement.scrollIntoView();
+      document.body.setAttribute(attrAutoScrollRunning, 'true');
       window.location.hash = '';
-      setTimeout(() => { window.location.hash = currentHash }, 0);
+      window.location.hash = currentHash;
+      setTimeout(() => document.body.removeAttribute(attrAutoScrollRunning), 0);
       return;
     }
 
     // If looking for a hidden comment, try to expand the section that contains it
-    if (currentHash.startsWith('#issuecomment-')) {
-      const commentId = currentHash.substring('#issuecomment-'.length);
+    const issueCommentPrefix = '#issuecomment-';
+    if (currentHash.startsWith(issueCommentPrefix)) {
+      const commentId = currentHash.substring(issueCommentPrefix.length);
       const expandButton = findExpandButtonForComment(commentId);
       if (expandButton) {
-        // Avoid infinite loop, do not re-click the button if already clicked
-        if (expandButton.hasAttribute('data-auto-load-clicked')) return;
-        expandButton.setAttribute('data-auto-load-clicked', 'true');
+        // avoid infinite loop, do not re-click the button if already clicked
+        const attrAutoLoadClicked = 'data-auto-load-clicked';
+        if (expandButton.hasAttribute(attrAutoLoadClicked)) return;
+        expandButton.setAttribute(attrAutoLoadClicked, 'true');
         expandButton.click();
         await sleep(500); // Wait for HTMX to load the content. FIXME: need to drop htmx in the future
         continue; // Try again to find the element
@@ -276,8 +285,8 @@ function findExpandButtonForComment(commentId: string): HTMLElement | null {
 }
 
 function initRepoDiffHashChangeListener() {
-  window.addEventListener('hashchange', loadUntilFound);
-  loadUntilFound();
+  window.addEventListener('hashchange', onLocationHashChange);
+  onLocationHashChange();
 }
 
 export function initRepoDiffView() {