[gh] Fix ranges calculation for GitHub multiline comments (IJPL-208090)

#IJPL-208090 Fixed

(cherry picked from commit 15c263c41eb70aee2dbeb13429800130737ef494)


(cherry picked from commit 19c92cc7b3b7950fdd2b8c17cd4e1ca9f9f577c9)

IJ-CR-179930

GitOrigin-RevId: e2577091d429869667f1b9377082e0db01622b97

plugins/github/github-core/src/org/jetbrains/plugins/github/api/data/pullrequest/GHPullRequestReviewThread.kt

@@ -73,83 +73,142 @@ fun GHPullRequestReviewThread.isVisible(viewOption: DiscussionsViewOption): Bool
     DiscussionsViewOption.DONT_SHOW -> false
   }
 
+private enum class StartOrEnd {
+  START, END;
+}
+
+private data class LineOnCommit(
+  val commitSha: String,
+  val lineIndex: Int,
+)
+
 fun GHPullRequestReviewThread.mapToLeftSideLine(diffData: GitTextFilePatchWithHistory): Int? =
   mapToSidedLine(diffData, Side.LEFT)
 
 fun GHPullRequestReviewThread.mapToRightSideLine(diffData: GitTextFilePatchWithHistory): Int? =
   mapToSidedLine(diffData, Side.RIGHT)
 
-private fun GHPullRequestReviewThread.mapToSidedLine(diffData: GitTextFilePatchWithHistory, side: Side): Int? {
-  val threadData = this
-  if (threadData.line == null && threadData.originalLine == null) return null
-
-  val lineIndex = threadData.line ?: threadData.originalLine ?: return null
-  val fromCommitSha = fromCommitSha(diffData) ?: return null
-
-  return diffData.forcefullyMapLine(fromCommitSha, lineIndex - 1, side)
+private fun GHPullRequestReviewThread.mapToSidedLine(
+  diffData: GitTextFilePatchWithHistory,
+  side: Side,
+): Int? {
+  val (fromCommit, lineIndex) = lineOnCommit(diffData, StartOrEnd.END, side) ?: return null
+  return diffData.forcefullyMapLine(fromCommit, lineIndex - 1, side)
 }
 
-// TODO: Write tests to illustrate and check the working of location mapping :'(
-fun GHPullRequestReviewThread.mapToLocation(diffData: GitTextFilePatchWithHistory, sideBias: Side? = null): DiffLineLocation? {
-  val threadData = this
-  if (threadData.line == null && threadData.originalLine == null) return null
+fun GHPullRequestReviewThread.mapToRange(
+  diffData: GitTextFilePatchWithHistory,
+  sideBias: Side = Side.LEFT,
+): Pair<Side, IntRange>? {
+  val (initialEndSide, initialEndLine) = mapToLocation(diffData, StartOrEnd.END, sideBias) ?: return null
 
-  val lineIndex = threadData.line ?: threadData.originalLine ?: return null
-  val fromCommitSha = fromCommitSha(diffData) ?: return null
-
-  val sideBias = sideBias ?: threadData.side
-
-  return diffData.mapLine(fromCommitSha, lineIndex - 1, sideBias)
-}
-
-fun GHPullRequestReviewThread.getCommentRange(diffData: GitTextFilePatchWithHistory): Pair<Side, IntRange>? {
-  val threadData = this
-  val fromSha = fromCommitSha(diffData) ?: return null
-
-  val (side, endLine) = mapToLocation(diffData) ?: return null
-
-  val unmappedStartLine = threadData.startLine ?: threadData.originalStartLine ?: return side to endLine..endLine
-  val startLine = diffData.forcefullyMapLine(fromSha, unmappedStartLine - 1, side) ?: return null
-
-  return side to if (startLine <= endLine) {
-    startLine..endLine
+  // there is no startLine, we are done mapping
+  if (startLine == null && originalStartLine == null) {
+    return initialEndSide to initialEndLine..initialEndLine
   }
-  else {
-    LOG.warn("Invalid comment range lines: $startLine..$endLine")
-    endLine..startLine
-  }
-}
 
-
-fun GHPullRequestReviewThread.getInEditorCommentRange(diffData: GitTextFilePatchWithHistory): Pair<Side, IntRange>? {
-  val threadData = this
-  val fromCommitSha = fromCommitSha(diffData) ?: return null
-  val endLine = threadData.mapToRightSideLine(diffData) ?: return null
-  val side = Side.RIGHT
-  val unmappedStartLine = threadData.startLine ?: threadData.originalStartLine ?: return side to endLine..endLine
-  val startLine = diffData.forcefullyMapLine(fromCommitSha, unmappedStartLine - 1, side) ?: return side to endLine..endLine
-  return side to if (startLine <= endLine) {
-    startLine..endLine
-  }
-  else {
-    LOG.warn("Invalid comment range lines: $startLine..$endLine")
-    endLine..startLine
-  }
-}
-
-private fun GHPullRequestReviewThread.fromCommitSha(diffData: GitTextFilePatchWithHistory): String? {
-  val threadData = this
-
-  return if (threadData.line != null) when (threadData.side) {
-    Side.RIGHT -> threadData.commit?.oid
-    Side.LEFT -> diffData.fileHistory.findStartCommit()
-  }
-  else if (threadData.originalLine != null) {
-    val originalCommitSha = threadData.originalCommit?.oid ?: return null
-    when (threadData.side) {
-      Side.RIGHT -> originalCommitSha
-      Side.LEFT -> diffData.fileHistory.findFirstParent(originalCommitSha)
+  val (initialStartSide, initialStartLine) = mapToLocation(diffData, StartOrEnd.START, initialEndSide) ?: return null
+  val (side, startLine, endLine) =
+    if (initialStartSide == initialEndSide) {
+      Triple(initialStartSide, initialStartLine, initialEndLine)
     }
+    else {
+      // cannot map startLine to the same side as endLine
+      if (initialEndSide != sideBias) return null
+
+      // otherwise, try to map the endLine to the same side as startLine
+      val END = mapToLocation(diffData, StartOrEnd.END, initialStartSide)
+                  ?.takeIf { (endSide, _) -> endSide == initialStartSide }?.second ?: return null
+
+      Triple(initialStartSide, initialStartLine, END)
+    }
+
+  return side to if (startLine <= endLine) {
+    startLine..endLine
+  }
+  else {
+    LOG.warn("Invalid comment range lines: $startLine..$endLine")
+    endLine..startLine
   }
-  else null
+}
+
+private fun GHPullRequestReviewThread.mapToLocation(
+  diffData: GitTextFilePatchWithHistory,
+  startOrEnd: StartOrEnd,
+  sideBias: Side,
+): DiffLineLocation? {
+  val (commit, lineIndex) = lineOnCommit(diffData, startOrEnd, sideBias) ?: return null
+  return diffData.mapLine(commit, lineIndex - 1, sideBias)
+}
+
+
+fun GHPullRequestReviewThread.mapToInEditorRange(diffData: GitTextFilePatchWithHistory): IntRange? {
+  val threadData = this
+
+  // already on latest and there's a mapped line, then use that one
+  if (
+    threadData.side == Side.RIGHT &&
+    diffData.patch.afterVersionId == threadData.commit?.oid &&
+    threadData.line != null
+  ) {
+    val startLineIndex = (threadData.startLine ?: threadData.line) - 1
+    val endLineIndex = threadData.line - 1
+
+    return startLineIndex..endLineIndex
+  }
+
+  return threadData.mapToRange(diffData, sideBias = Side.RIGHT)
+    ?.takeIf { (side, _) -> side == Side.RIGHT }?.second
+}
+
+/**
+ * @param sideBias Indicates what side we would prefer the lines to be mapped to.
+ * We choose the line and commit that are closest to the preferred side.
+ */
+private fun GHPullRequestReviewThread.lineOnCommit(
+  diffData: GitTextFilePatchWithHistory,
+  startOrEnd: StartOrEnd,
+  sideBias: Side,
+): LineOnCommit? {
+  val (unmappedLine, unmappedOriginalLine) = when (startOrEnd) {
+    StartOrEnd.END -> line to originalLine
+    StartOrEnd.START -> startLine to originalStartLine
+  }
+
+  fun mapOriginalLine() =
+    toLineOnCommit(path, diffData, side, originalCommit?.oid, unmappedOriginalLine)
+
+  fun mapLine() =
+    toLineOnCommit(path, diffData, side, commit?.oid, unmappedLine)
+
+  return when (sideBias) {
+    Side.LEFT -> mapOriginalLine() ?: mapLine()
+    Side.RIGHT -> mapLine() ?: mapOriginalLine()
+  }
+}
+
+private fun toLineOnCommit(
+  file: String,
+  diffData: GitTextFilePatchWithHistory,
+  side: Side?,
+  commitSha: String?,
+  lineIndex: Int?,
+): LineOnCommit? {
+  val side = side ?: return null
+  val commitSha = commitSha ?: return null
+  val lineIndex = lineIndex ?: return null
+
+  if (!diffData.contains(commitSha, file)) return null
+
+  return LineOnCommit(
+    when (side) {
+      Side.RIGHT -> commitSha
+      Side.LEFT ->
+        (
+          if (diffData.isCumulative) diffData.fileHistory.findStartCommit()
+          else diffData.patch.beforeVersionId
+        ) ?: return null
+    },
+    lineIndex,
+  )
 }

plugins/github/github-core/src/org/jetbrains/plugins/github/pullrequest/ui/diff/GHPRDiffViewModel.kt

@@ -21,8 +21,8 @@ import kotlinx.coroutines.launch
 import org.jetbrains.annotations.ApiStatus
 import org.jetbrains.plugins.github.api.data.GHUser
 import org.jetbrains.plugins.github.api.data.pullrequest.GHPullRequestReviewThread
-import org.jetbrains.plugins.github.api.data.pullrequest.getCommentRange
 import org.jetbrains.plugins.github.api.data.pullrequest.isVisible
+import org.jetbrains.plugins.github.api.data.pullrequest.mapToRange
 import org.jetbrains.plugins.github.pullrequest.config.GithubPullRequestsProjectUISettings
 import org.jetbrains.plugins.github.pullrequest.data.GHPRDataContext
 import org.jetbrains.plugins.github.pullrequest.data.provider.GHPRDataProvider
@@ -145,7 +145,7 @@ internal class GHPRDiffViewModelImpl(
                          ?: return@associateBy GHPRReviewThreadDiffViewModel.MappingData(isVisible, null, null)
             val diffData = allChanges.patchesByChange[change]
                            ?: return@associateBy GHPRReviewThreadDiffViewModel.MappingData(isVisible, change, null)
-            val commentRange = threadData.getCommentRange(diffData)
+            val commentRange = threadData.mapToRange(diffData)
             GHPRReviewThreadDiffViewModel.MappingData(isVisible, change, commentRange)
           }
         }

plugins/github/github-core/src/org/jetbrains/plugins/github/pullrequest/ui/editor/GHPRReviewInEditorViewModel.kt

@@ -20,8 +20,8 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.awaitCancellation
 import kotlinx.coroutines.coroutineScope
 import kotlinx.coroutines.flow.*
-import org.jetbrains.plugins.github.api.data.pullrequest.getInEditorCommentRange
 import org.jetbrains.plugins.github.api.data.pullrequest.isVisible
+import org.jetbrains.plugins.github.api.data.pullrequest.mapToInEditorRange
 import org.jetbrains.plugins.github.pullrequest.config.GithubPullRequestsProjectUISettings
 import org.jetbrains.plugins.github.pullrequest.data.GHPRDataContext
 import org.jetbrains.plugins.github.pullrequest.data.provider.GHPRDataProvider
@@ -72,7 +72,7 @@ internal class GHPRReviewInEditorViewModelImpl(
 
         val diffData = mappingData.diffData
                        ?: return@mapValues MappedGHPRReviewEditorThreadViewModel.MappingData(isVisible, mappingData.change, null)
-        val commentRange = mappingData.threadData.getInEditorCommentRange(diffData)?.second
+        val commentRange = mappingData.threadData.mapToInEditorRange(diffData)
 
         MappedGHPRReviewEditorThreadViewModel.MappingData(isVisible, mappingData.change, commentRange)
       }

plugins/github/github-core/test/org/jetbrains/plugins/github/api/data/pullrequest/GHPullRequestReviewThreadTest.kt

@@ -0,0 +1,343 @@
+// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
+package org.jetbrains.plugins.github.api.data.pullrequest
+
+import com.intellij.collaboration.api.dto.GraphQLCursorPageInfoDTO
+import com.intellij.collaboration.api.dto.GraphQLNodesDTO
+import com.intellij.collaboration.ui.codereview.diff.DiffLineLocation
+import com.intellij.diff.util.Side
+import git4idea.changes.GitTextFilePatchWithHistory
+import io.mockk.every
+import io.mockk.mockk
+import org.assertj.core.api.Assertions.assertThat
+import org.jetbrains.plugins.github.api.data.GHCommitHash
+import org.jetbrains.plugins.github.api.data.GHReactable
+import org.jetbrains.plugins.github.api.data.pullrequest.GHPullRequestReviewThreadTest.Util.Commit
+import org.junit.jupiter.api.Test
+import java.util.*
+
+class GHPullRequestReviewThreadTest {
+  @Test
+  fun `mapToInEditorRange uses the current line numbers if already mapped`() {
+    val thread = Util.createPRThread(
+      startLine = 5, originalStartLine = 2,
+      line = 6, originalLine = 3,
+      commit = Commit.Last, originalCommit = Commit.Commit1,
+      side = Side.RIGHT,
+    )
+
+    val diffData = Util.mockDiffData()
+
+    assertThat(thread.mapToInEditorRange(diffData))
+      .isEqualTo(5..6)
+  }
+
+  @Test
+  fun `mapToInEditorRange tries to map current line numbers if not mapped to last commit`() {
+    val thread = Util.createPRThread(
+      startLine = 5, originalStartLine = 2,
+      line = 6, originalLine = 3,
+      commit = Commit.Commit2, originalCommit = Commit.Commit1,
+      side = Side.RIGHT,
+    )
+
+    val diffData = Util.mockDiffData(mapping = { commit, side, lineIndex ->
+      if (commit == Commit.Commit2 && side == Side.RIGHT)
+        side to (lineIndex + 5)
+      else error("unexpected")
+    })
+
+    assertThat(thread.mapToInEditorRange(diffData))
+      .isEqualTo(10..11)
+  }
+
+  @Test
+  fun `mapToInEditorRange produces no range if it's mapped to the left`() {
+    val thread = Util.createPRThread(
+      startLine = null, originalStartLine = 2,
+      line = null, originalLine = 3,
+      commit = Commit.Commit2, originalCommit = Commit.Commit1,
+      side = Side.RIGHT,
+    )
+
+    val diffData = Util.mockDiffData(mapping = { commit, _, lineIndex ->
+      // represents that we can only map to the left side
+      if (commit == Commit.Commit1)
+        Side.LEFT to (lineIndex - 1)
+      else error("unexpected")
+    })
+
+    assertThat(thread.mapToInEditorRange(diffData))
+      .isNull()
+  }
+
+  @Test
+  fun `mapToRange can recover from startLine failing to map to the bias side`() {
+    val thread = Util.createPRThread(
+      startLine = null, originalStartLine = 10,
+      line = null, originalLine = 15,
+      commit = Commit.Commit2, originalCommit = Commit.Commit1,
+      side = Side.RIGHT,
+    )
+
+    val diffData = Util.mockDiffData(mapping = { commit, side, lineIndex ->
+      if (commit != Commit.Commit1) return@mockDiffData null
+
+      when (lineIndex) {
+        // the endLine can be mapped to LEFT OR RIGHT
+        15 if side == Side.LEFT -> Side.LEFT to (lineIndex - 1)
+        15 if side == Side.RIGHT -> Side.RIGHT to (lineIndex + 1)
+        // but startLine can only be mapped to the LEFT
+        10 -> Side.LEFT to (lineIndex - 1)
+        else -> error("unexpected")
+      }
+    })
+
+    assertThat(thread.mapToRange(diffData, Side.RIGHT))
+      .isEqualTo(Side.LEFT to (9..14))
+  }
+
+  @Test
+  fun `mapToRange can also prefer the left side`() {
+    val thread = Util.createPRThread(
+      startLine = null, originalStartLine = 10,
+      line = null, originalLine = 15,
+      commit = Commit.Commit2, originalCommit = Commit.Commit1,
+      side = Side.RIGHT,
+    )
+
+    val diffData = Util.mockDiffData(mapping = { commit, side, lineIndex ->
+      if (commit != Commit.Commit1) return@mockDiffData null
+
+      when (lineIndex) {
+        // the endLine can be mapped to LEFT OR RIGHT
+        15 if side == Side.LEFT -> Side.LEFT to (lineIndex - 1)
+        15 if side == Side.RIGHT -> Side.RIGHT to (lineIndex + 1)
+        // but startLine can only be mapped to the LEFT
+        10 -> Side.LEFT to (lineIndex - 1)
+        else -> error("unexpected")
+      }
+    })
+
+    assertThat(thread.mapToRange(diffData, Side.LEFT))
+      .isEqualTo(Side.LEFT to (9..14))
+  }
+
+  @Test
+  fun `mapToRange can recover from endLine failing to map to the bias side`() {
+    val thread = Util.createPRThread(
+      startLine = null, originalStartLine = 10,
+      line = null, originalLine = 15,
+      commit = Commit.Commit2, originalCommit = Commit.Commit1,
+      side = Side.RIGHT,
+    )
+
+    val diffData = Util.mockDiffData(mapping = { commit, side, lineIndex ->
+      if (commit != Commit.Commit1) return@mockDiffData null
+
+      when (lineIndex) {
+        // the startLine can be mapped to LEFT OR RIGHT
+        10 if side == Side.LEFT -> Side.LEFT to (lineIndex - 1)
+        10 if side == Side.RIGHT -> Side.RIGHT to (lineIndex + 1)
+        // but endLine can only be mapped to the LEFT
+        15 -> Side.LEFT to (lineIndex - 1)
+        else -> error("unexpected")
+      }
+    })
+
+    assertThat(thread.mapToRange(diffData, Side.RIGHT))
+      .isEqualTo(Side.LEFT to (9..14))
+  }
+
+  @Test
+  fun `mapToRange just returns endLine to endLine range if no startLine is present`() {
+    val thread = Util.createPRThread(
+      startLine = null, originalStartLine = null,
+      line = null, originalLine = 15,
+      commit = Commit.Commit2, originalCommit = Commit.Commit1,
+      side = Side.RIGHT,
+    )
+
+    val diffData = Util.mockDiffData(mapping = { commit, side, lineIndex ->
+      if (commit != Commit.Commit1) return@mockDiffData null
+
+      when (lineIndex) {
+        // endLine can only be mapped to the LEFT
+        15 -> Side.LEFT to (lineIndex - 1)
+        else -> error("unexpected")
+      }
+    })
+
+    assertThat(thread.mapToRange(diffData, Side.RIGHT))
+      .isEqualTo(Side.LEFT to (14..14))
+  }
+
+  @Test
+  fun `mapToRange cannot recover from startLine and endLine failing to map to the same side 1`() {
+    val thread = Util.createPRThread(
+      startLine = null, originalStartLine = 10,
+      line = null, originalLine = 15,
+      commit = Commit.Commit2, originalCommit = Commit.Commit1,
+      side = Side.RIGHT,
+    )
+
+    val diffData = Util.mockDiffData(mapping = { commit, _, lineIndex ->
+      if (commit != Commit.Commit1) return@mockDiffData null
+
+      when (lineIndex) {
+        // endLine can only be mapped to the RIGHT
+        15 -> Side.RIGHT to (lineIndex + 1)
+        // startLine can only be mapped to the LEFt
+        10 -> Side.LEFT to (lineIndex - 1)
+        else -> error("unexpected")
+      }
+    })
+
+    assertThat(thread.mapToRange(diffData, Side.LEFT))
+      .isNull()
+  }
+
+  @Test
+  fun `mapToRange cannot recover from startLine and endLine failing to map to the same side 2`() {
+    val thread = Util.createPRThread(
+      startLine = null, originalStartLine = 10,
+      line = null, originalLine = 15,
+      commit = Commit.Commit2, originalCommit = Commit.Commit1,
+      side = Side.RIGHT,
+    )
+
+    val diffData = Util.mockDiffData(mapping = { commit, _, lineIndex ->
+      if (commit != Commit.Commit1) return@mockDiffData null
+
+      when (lineIndex) {
+        // endLine can only be mapped to the LEFT
+        15 -> Side.LEFT to (lineIndex - 1)
+        // startLine can only be mapped to the RIGHT
+        10 -> Side.RIGHT to (lineIndex + 1)
+        else -> error("unexpected")
+      }
+    })
+
+    assertThat(thread.mapToRange(diffData, Side.LEFT))
+      .isNull()
+  }
+
+  //region: Util
+  private object Util {
+    enum class Commit(val sha: String) {
+      // represents the commit before the PR changes
+      Before("before"),
+
+      // the first commit actually in the PR
+      Commit1("commit1"),
+      Commit2("commit2"),
+      Commit3("commit3");
+
+      companion object {
+        // the last commit actually in the PR
+        val Last: Commit = Commit3
+      }
+    }
+
+    private const val PATH = "file.txt"
+
+    fun mockDiffData(
+      beforeCommit: Commit = Commit.Before,
+      lastCommit: Commit = Commit.Last,
+
+      mapping: (Commit, Side, Int) -> DiffLineLocation? = { _, _, _ -> null },
+      forcefulMapping: (Commit, Side, Int) -> Int? = { _, _, _ -> null },
+    ): GitTextFilePatchWithHistory = mockk<GitTextFilePatchWithHistory> {
+      val mock = this
+
+      every {
+        mock.contains(any(), any())
+      } answers {
+        val commitSha = args[0] as String
+        val file = args[1] as String
+
+        val idx = Commit.entries.find { it.sha == commitSha }!!.ordinal
+        beforeCommit.ordinal <= idx && idx <= lastCommit.ordinal && file == PATH
+      }
+
+      every {
+        mock.patch.beforeVersionId
+      } returns beforeCommit.sha
+      every {
+        mock.patch.afterVersionId
+      } returns lastCommit.sha
+
+      every {
+        mock.mapLine(any(), any(), any())
+      } answers {
+        val fromCommit = Commit.entries.find { it.sha == args[0] as String }!!
+        val lineIndex = args[1] as Int
+        val side = args[2] as Side
+
+        mapping(fromCommit, side, lineIndex)
+      }
+
+      every {
+        mock.forcefullyMapLine(any(), any(), any())
+      } answers {
+        val fromCommit = Commit.entries.find { it.sha == args[0] as String }!!
+        val lineIndex = args[1] as Int
+        val side = args[2] as Side
+
+        forcefulMapping(fromCommit, side, lineIndex)
+      }
+    }
+
+    /**
+     * All line numbers are 0-based for ease of reading in tests.
+     */
+    fun createPRThread(
+      line: Int? = null,
+      originalLine: Int? = null,
+
+      startLine: Int? = null,
+      originalStartLine: Int? = null,
+
+      side: Side = Side.RIGHT,
+      startSide: Side = side,
+
+      commit: Commit? = null,
+      originalCommit: Commit? = null,
+    ): GHPullRequestReviewThread = GHPullRequestReviewThread(
+      id = "",
+      isResolved = false,
+      isOutdated = false,
+      path = PATH,
+      side = side,
+      line = line?.plus(1),
+      originalLine = originalLine?.plus(1),
+      startSide = startSide,
+      startLine = startLine?.plus(1),
+      originalStartLine = originalStartLine?.plus(1),
+      commentsNodes = GraphQLNodesDTO(
+        nodes = listOf(
+          GHPullRequestReviewComment(
+            id = "",
+            url = "",
+            author = null,
+            body = "Some text",
+            createdAt = Date(),
+            reactions = GHReactable.ReactionConnection(
+              GraphQLCursorPageInfoDTO("", false, "", false),
+            ),
+            state = GHPullRequestReviewCommentState.SUBMITTED,
+            commit = commit?.sha?.let { GHCommitHash("", it, it.take(7)) },
+            originalCommit = originalCommit?.sha?.let { GHCommitHash("", it, it.take(7)) },
+            diffHunk = "",
+            pullRequestReview = null,
+            viewerCanDelete = true,
+            viewerCanUpdate = true,
+            viewerCanReact = true,
+          )
+        )),
+      viewerCanReply = true,
+      viewerCanResolve = true,
+      viewerCanUnresolve = true,
+    )
+  }
+  //endregion
+}