diff --git a/plugins/github/github-core/src/org/jetbrains/plugins/github/api/data/pullrequest/GHPullRequestReviewThread.kt b/plugins/github/github-core/src/org/jetbrains/plugins/github/api/data/pullrequest/GHPullRequestReviewThread.kt index b534cd9c448b..a911bff91f40 100644 --- a/plugins/github/github-core/src/org/jetbrains/plugins/github/api/data/pullrequest/GHPullRequestReviewThread.kt +++ b/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? { + 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? { - 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? { - 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, + ) } diff --git a/plugins/github/github-core/src/org/jetbrains/plugins/github/pullrequest/ui/diff/GHPRDiffViewModel.kt b/plugins/github/github-core/src/org/jetbrains/plugins/github/pullrequest/ui/diff/GHPRDiffViewModel.kt index 19608c6b47ac..fe76c919fdcf 100644 --- a/plugins/github/github-core/src/org/jetbrains/plugins/github/pullrequest/ui/diff/GHPRDiffViewModel.kt +++ b/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) } } diff --git a/plugins/github/github-core/src/org/jetbrains/plugins/github/pullrequest/ui/editor/GHPRReviewInEditorViewModel.kt b/plugins/github/github-core/src/org/jetbrains/plugins/github/pullrequest/ui/editor/GHPRReviewInEditorViewModel.kt index 6c2e5ae1feee..faadfb0fff0a 100644 --- a/plugins/github/github-core/src/org/jetbrains/plugins/github/pullrequest/ui/editor/GHPRReviewInEditorViewModel.kt +++ b/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) } diff --git a/plugins/github/github-core/test/org/jetbrains/plugins/github/api/data/pullrequest/GHPullRequestReviewThreadTest.kt b/plugins/github/github-core/test/org/jetbrains/plugins/github/api/data/pullrequest/GHPullRequestReviewThreadTest.kt new file mode 100644 index 000000000000..529e6a6dfda9 --- /dev/null +++ b/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 { + 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 +} \ No newline at end of file