IJPL-237452 [gitlab]: forbid to adjust commit line ranges if server does not support it

GitOrigin-RevId: a4f14f41d8c2656d71934256d787eb075e675eb4
This commit is contained in:
Valeria Golovina
2026-03-09 13:08:26 +00:00
committed by intellij-monorepo-bot
parent 4c945f894e
commit 5a99e6083a
14 changed files with 58 additions and 20 deletions
@@ -1059,6 +1059,7 @@ f:com.intellij.collaboration.ui.codereview.diff.viewer.DiffViewerUtilKt
- java.lang.Enum
- sf:SINGLE_COMMIT_REVIEW:com.intellij.collaboration.ui.codereview.editor.CodeReviewInlayModel$Ranged$Adjustable$AdjustmentDisabledReason
- sf:SUGGESTED_CHANGE:com.intellij.collaboration.ui.codereview.editor.CodeReviewInlayModel$Ranged$Adjustable$AdjustmentDisabledReason
- sf:UNSUPPORTED_VERSION:com.intellij.collaboration.ui.codereview.editor.CodeReviewInlayModel$Ranged$Adjustable$AdjustmentDisabledReason
- s:getEntries():kotlin.enums.EnumEntries
- s:valueOf(java.lang.String):com.intellij.collaboration.ui.codereview.editor.CodeReviewInlayModel$Ranged$Adjustable$AdjustmentDisabledReason
- s:values():com.intellij.collaboration.ui.codereview.editor.CodeReviewInlayModel$Ranged$Adjustable$AdjustmentDisabledReason[]
@@ -182,6 +182,7 @@ review.comments.discard.new.confirmation=Are you sure you want to discard the un
review.comments.code.outline.tooltip.explanation=Drag to select several lines for commenting
review.comments.code.outline.tooltip.suggestion.disabling=The range of this comment is not adjustable as it contains suggestion
review.comments.code.outline.tooltip.commit.review.disabling=Comment range cannot be adjusted while reviewing a single commit
review.comments.code.outline.tooltip.version.not.supported.disabling=Comment range adjustment requires a newer server version
review.comment.new.line.hint={0} to add new line
review.comment.save=Save Changes
@@ -369,6 +369,9 @@ private class ResizableOutlineHandler private constructor(
AdjustmentDisabledReason.SINGLE_COMMIT_REVIEW -> {
tooltipManager.showTooltip(component, point, OutlineTooltipManager.TooltipReason.SINGLE_COMMIT_REVIEW)
}
AdjustmentDisabledReason.UNSUPPORTED_VERSION -> {
tooltipManager.showTooltip(component, point, OutlineTooltipManager.TooltipReason.UNSUPPORTED_VERSION)
}
else -> {
tooltipManager.showTooltip(component, point, OutlineTooltipManager.TooltipReason.MLC_EXPLANATION)
setEditorCursor(resizeCursor)
@@ -438,13 +441,15 @@ private class OutlineTooltipManager(private val editor: Editor) {
enum class TooltipReason {
SUGGESTION,
MLC_EXPLANATION,
SINGLE_COMMIT_REVIEW;
SINGLE_COMMIT_REVIEW,
UNSUPPORTED_VERSION;
companion object {
fun getTooltipMessage(tooltipReason: TooltipReason) = when (tooltipReason) {
SUGGESTION -> CollaborationToolsBundle.message("review.comments.code.outline.tooltip.suggestion.disabling")
MLC_EXPLANATION -> CollaborationToolsBundle.message("review.comments.code.outline.tooltip.explanation")
SINGLE_COMMIT_REVIEW -> CollaborationToolsBundle.message("review.comments.code.outline.tooltip.commit.review.disabling")
UNSUPPORTED_VERSION -> CollaborationToolsBundle.message("review.comments.code.outline.tooltip.version.not.supported.disabling")
}
}
}
@@ -28,6 +28,7 @@ interface CodeReviewInlayModel : EditorMappedViewModel {
enum class AdjustmentDisabledReason {
SUGGESTED_CHANGE,
SINGLE_COMMIT_REVIEW,
UNSUPPORTED_VERSION,
}
}
}
@@ -85,11 +85,12 @@ suspend fun GitLabApi.Rest.addDiffNote(
projectId: String,
mrIid: String,
position: GitLabDiffPositionInput,
canPostPositionLineRange: Boolean,
body: String,
): HttpResponse<out GitLabDiscussionRestDTO> {
val uri = getMergeRequestDiscussionsUri(projectId, mrIid).withQuery {
"body" eq body
addDiffPositionParameters(position)
addDiffPositionParameters(position, canPostPositionLineRange)
}
val request = request(uri).POST(HttpRequest.BodyPublishers.noBody()).build()
return withErrorStats(GitLabApiRequestName.REST_CREATE_MERGE_REQUEST_DIFF_NOTE) {
@@ -179,7 +180,8 @@ suspend fun GitLabApi.Rest.changeMergeRequestDiscussionResolve(
* This function converts a [GitLabDiffPositionInput] to query parameters in the format
* expected by GitLab REST API for creating notes and draft notes on diffs.
*/
internal fun GitLabApiUriQueryBuilder.addDiffPositionParameters(position: GitLabDiffPositionInput) {
internal fun GitLabApiUriQueryBuilder.addDiffPositionParameters(position: GitLabDiffPositionInput,
canPostPositionLineRange: Boolean) {
"position" {
"base_sha" eq position.baseSha
"head_sha" eq position.headSha
@@ -190,12 +192,16 @@ internal fun GitLabApiUriQueryBuilder.addDiffPositionParameters(position: GitLab
"old_path" eq position.paths.oldPath
"position_type" eq "text"
position.lineRange?.let { lineRange ->
addLineRangeParameters(lineRange)
addLineRangeParameters(lineRange, canPostPositionLineRange)
}
}
}
internal fun GitLabApiUriQueryBuilder.addLineRangeParameters(lineRange: LineRangeDTO) {
internal fun GitLabApiUriQueryBuilder.addLineRangeParameters(lineRange: LineRangeDTO,
canPostPositionLineRange: Boolean) {
// providing line_range in API call will cause GitLab to return 400 Bad Request
// if the position is not supported by GitLab server, so we just omit it in this case
if (!canPostPositionLineRange) return
"line_range" {
"start" {
"line_code" eq lineRange.start.lineCode
@@ -37,11 +37,12 @@ suspend fun GitLabApi.Rest.updateDraftNote(
noteId: String,
position: GitLabMergeRequestDraftNoteRestDTO.Position,
body: String,
isMultilinePositionSupported: Boolean,
)
: HttpResponse<out Unit> {
val uri = getSpecificMergeRequestDraftNoteUri(projectId, mrIid, noteId).withQuery {
"note" eq body
addDraftNotePositionParameters(position) // have to pass the existing position, otherwise it is reset to null
addDraftNotePositionParameters(position, isMultilinePositionSupported) // have to pass the existing position, otherwise it is reset to null
}
val request = request(uri).PUT(BodyPublishers.noBody()).build()
return withErrorStats(GitLabApiRequestName.REST_UPDATE_DRAFT_NOTE) {
@@ -112,11 +113,12 @@ suspend fun GitLabApi.Rest.addDraftNote(
mrIid: String,
@SinceGitLab("16.3")
positionOrNull: GitLabDiffPositionInput?,
canPostPositionLineRange: Boolean,
body: String,
): HttpResponse<out GitLabMergeRequestDraftNoteRestDTO> {
val uri = getMergeRequestDraftNotesUri(projectId, mrIid).withQuery {
"note" eq body
positionOrNull?.let { addDiffPositionParameters(it) }
positionOrNull?.let { addDiffPositionParameters(it, canPostPositionLineRange) }
}
val request = request(uri).POST(BodyPublishers.noBody()).build()
return withErrorStats(GitLabApiRequestName.REST_CREATE_DRAFT_NOTE) {
@@ -124,7 +126,10 @@ suspend fun GitLabApi.Rest.addDraftNote(
}
}
private fun GitLabApiUriQueryBuilder.addDraftNotePositionParameters(position: GitLabMergeRequestDraftNoteRestDTO.Position) {
private fun GitLabApiUriQueryBuilder.addDraftNotePositionParameters(
position: GitLabMergeRequestDraftNoteRestDTO.Position,
canPostPositionLineRange: Boolean
) {
// If there's no position info (just position type), don't pass it to GitLab.
if (position.baseSha == null && position.headSha == null && position.startSha == null &&
position.newPath == null && position.oldPath == null &&
@@ -142,7 +147,7 @@ private fun GitLabApiUriQueryBuilder.addDraftNotePositionParameters(position: Gi
"new_line" eq position.newLine
"position_type" eq position.positionType
position.lineRange?.let { lineRange ->
addLineRangeParameters(lineRange)
addLineRangeParameters(lineRange, canPostPositionLineRange)
}
}
}
@@ -429,6 +429,7 @@ internal class LoadedGitLabMergeRequest(
override val canAddNotes: Boolean = discussionsContainer.canAddNotes
override val canAddDraftNotes: Boolean = discussionsContainer.canAddDraftNotes
override val canAddPositionalDraftNotes: Boolean = discussionsContainer.canAddPositionalDraftNotes
override val canAddMultilinePositionalNotes: Boolean = discussionsContainer.canAddMultilinePositionalNotes
override suspend fun addNote(body: String) =
discussionsContainer.addNote(body)
@@ -53,6 +53,10 @@ interface GitLabMergeRequestDiscussionsContainer {
val canAddNotes: Boolean
val canAddDraftNotes: Boolean
val canAddPositionalDraftNotes: Boolean
/**
* if the position of a note can be multiline
*/
val canAddMultilinePositionalNotes: Boolean
suspend fun addNote(body: String)
@@ -79,11 +83,18 @@ class GitLabMergeRequestDiscussionsContainerImpl(
private val cs = parentCs.childScope(this::class, Dispatchers.Default)
override val canAddNotes: Boolean = mr.details.value.userPermissions.createNote
override val canAddDraftNotes: Boolean =
canAddNotes && (glMetadata != null && GitLabVersion(15, 10) <= glMetadata.version)
override val canAddPositionalDraftNotes: Boolean =
canAddNotes && (glMetadata != null && GitLabVersion(16, 3) <= glMetadata.version)
// There were two bugs in Gitlab api that cause posting failures for multiline comments,
// https://gitlab.com/gitlab-org/gitlab/-/issues/520794 - affects plain comments, fixed in v17.10
// https://gitlab.com/gitlab-org/gitlab/-/issues/571619 - affects drafts, fixed in v18.6
// since one messages can be posted either as a comment or as a draft, we need them both to work with multiline correctly
override val canAddMultilinePositionalNotes: Boolean =
canAddNotes && (glMetadata != null && GitLabVersion(18, 6) <= glMetadata.version)
private val reloadRequests = MutableSharedFlow<Unit>(replay = 1, onBufferOverflow = BufferOverflow.DROP_OLDEST).apply {
tryEmit(Unit)
@@ -181,7 +192,7 @@ class GitLabMergeRequestDiscussionsContainerImpl(
mapDataToModel(
GitLabMergeRequestDraftNoteRestDTO::id,
{
GitLabMergeRequestDraftNoteImpl(this, api, glMetadata, projectId, mr, { draftNotesEvents.emit(it) }, it, currentUser)
GitLabMergeRequestDraftNoteImpl(this, api, glMetadata, projectId, mr, { draftNotesEvents.emit(it) }, it, currentUser, canAddMultilinePositionalNotes)
},
{ update(it) }
)
@@ -215,7 +226,7 @@ class GitLabMergeRequestDiscussionsContainerImpl(
override suspend fun addNote(position: GitLabMergeRequestNewDiscussionPosition, body: String) {
withContext(cs.coroutineContext) {
val newDiscussion = withContext(Dispatchers.IO) {
api.rest.addDiffNote(projectId, mr.iid, GitLabDiffPositionInput.from(position), body).body()
api.rest.addDiffNote(projectId, mr.iid, GitLabDiffPositionInput.from(position), canAddMultilinePositionalNotes, body).body()
}
withContext(NonCancellable) {
@@ -227,7 +238,7 @@ class GitLabMergeRequestDiscussionsContainerImpl(
override suspend fun addDraftNote(body: String) {
withContext(cs.coroutineContext) {
val newNote = withContext(Dispatchers.IO) {
api.rest.addDraftNote(projectId, mr.iid, null, body).body()
api.rest.addDraftNote(projectId, mr.iid, null, canAddMultilinePositionalNotes, body).body()
}
withContext(NonCancellable) {
@@ -239,7 +250,7 @@ class GitLabMergeRequestDiscussionsContainerImpl(
override suspend fun addDraftNote(position: GitLabMergeRequestNewDiscussionPosition, body: String) {
withContext(cs.coroutineContext) {
val newNote = withContext(Dispatchers.IO) {
api.rest.addDraftNote(projectId, mr.iid, GitLabDiffPositionInput.from(position), body).body()
api.rest.addDraftNote(projectId, mr.iid, GitLabDiffPositionInput.from(position), canAddMultilinePositionalNotes, body).body()
}
withContext(NonCancellable) {
@@ -244,7 +244,8 @@ class GitLabMergeRequestDraftNoteImpl(
private val mr: GitLabMergeRequest,
private val eventSink: suspend (Change<GitLabMergeRequestDraftNoteRestDTO>) -> Unit,
private val noteData: GitLabMergeRequestDraftNoteRestDTO,
override val author: GitLabUserDTO
override val author: GitLabUserDTO,
private val isMultilinePositionSupported: Boolean,
) : GitLabMergeRequestDraftNote, MutableGitLabNote {
private val cs = parentCs.childScope(this::class)
@@ -273,7 +274,7 @@ class GitLabMergeRequestDraftNoteImpl(
operationsGuard.withLock {
withContext(Dispatchers.IO) {
// Checked by canEdit
api.rest.updateDraftNote(projectId, mr.iid, noteData.id.restId, noteData.position, newText)
api.rest.updateDraftNote(projectId, mr.iid, noteData.id.restId, noteData.position, newText, isMultilinePositionSupported)
}
}
data.update { it.copy(note = newText) }
@@ -262,9 +262,9 @@ private class DiffEditorModel(
override val isVisible: StateFlow<Boolean> = MutableStateFlow(true)
override val range: StateFlow<LineRange?> = vm.location.mapState { it?.toLineRange(locationToLine) }
override val line: StateFlow<Int?> = range.mapState { it?.end }
override val adjustmentDisabledReason = MutableStateFlow(
AdjustmentDisabledReason.SINGLE_COMMIT_REVIEW.takeIf { !diffReviewVm.isCumulativeChange }
)
override val adjustmentDisabledReason =
MutableStateFlow(AdjustmentDisabledReason.UNSUPPORTED_VERSION.takeIf { !vm.isMultilinePositionSupported }
?: AdjustmentDisabledReason.SINGLE_COMMIT_REVIEW.takeIf { !diffReviewVm.isCumulativeChange })
override fun adjustRange(newStart: Int?, newEnd: Int?) {
if (newStart == null && newEnd == null) return
@@ -65,7 +65,7 @@ class GitLabMergeRequestDiffNewDiscussionViewModel internal constructor(
private val base: NewGitLabNoteViewModelWithAdjustablePosition,
private val diffData: GitTextFilePatchWithHistory,
discussionsViewOption: StateFlow<DiscussionsViewOption>
) : NewGitLabNoteViewModel by base {
) : NewGitLabNoteViewModelWithAdjustablePosition by base {
val location: StateFlow<GitLabNoteLocation?> = base.position.mapState { it.mapToLocation(diffData) }
val isVisible: StateFlow<Boolean> = discussionsViewOption.mapState { it != DiscussionsViewOption.DONT_SHOW }
fun updateLineRange(startLocation: DiffLineLocation, endLocation: DiffLineLocation) {
@@ -12,6 +12,7 @@ import com.intellij.collaboration.ui.codereview.editor.CodeReviewEditorGutterAct
import com.intellij.collaboration.ui.codereview.editor.CodeReviewEditorGutterChangesModel
import com.intellij.collaboration.ui.codereview.editor.CodeReviewEditorGutterControlsModel
import com.intellij.collaboration.ui.codereview.editor.CodeReviewEditorInlaysModel
import com.intellij.collaboration.ui.codereview.editor.CodeReviewInlayModel.Ranged.Adjustable.AdjustmentDisabledReason
import com.intellij.collaboration.ui.codereview.editor.CodeReviewNavigableEditorViewModel
import com.intellij.collaboration.ui.codereview.diff.DiffLineLocation
import com.intellij.collaboration.ui.codereview.editor.MutableCodeReviewEditorGutterChangesModel
@@ -239,7 +240,8 @@ internal class GitLabMergeRequestEditorReviewUIModel internal constructor(
override val isVisible: StateFlow<Boolean> = MutableStateFlow(true)
override val range: StateFlow<LineRange?> = vm.location.shiftLineRange()
override val line: StateFlow<Int?> = range.mapState { it?.end }
override val adjustmentDisabledReason = MutableStateFlow(null)
override val adjustmentDisabledReason =
MutableStateFlow(AdjustmentDisabledReason.UNSUPPORTED_VERSION.takeIf { !vm.isMultilinePositionSupported })
override fun adjustRange(newStart: Int?, newEnd: Int?) {
if (newStart == null && newEnd == null) return
val ranges = postReviewRanges.value ?: emptyList()
@@ -154,6 +154,7 @@ interface NewGitLabNoteViewModel :
interface NewGitLabNoteViewModelWithAdjustablePosition : NewGitLabNoteViewModel {
val position: StateFlow<GitLabMergeRequestDiscussionsViewModels.NewDiscussionPosition>
val isMultilinePositionSupported: Boolean
fun updatePosition(newPosition: GitLabMergeRequestDiscussionsViewModels.NewDiscussionPosition)
}
@@ -216,6 +217,7 @@ private class NewDiffGitLabNoteViewModel(
private val _position = MutableStateFlow(position)
override val position = _position.asStateFlow()
override val canSubmitAsDraft: Boolean = mergeRequest.canAddPositionalDraftNotes
override val isMultilinePositionSupported: Boolean = mergeRequest.canAddMultilinePositionalNotes
override fun updatePosition(newPosition: GitLabMergeRequestDiscussionsViewModels.NewDiscussionPosition) {
_position.value = newPosition
}
@@ -207,6 +207,7 @@ class GitLabApiTest : GitLabApiTestCase() {
volatileProjectMr1Iid,
GitLabDiffPositionInput("bd857928", "bd857928", 1, "063282e5", 1,
DiffPathsInputDTO("README.md", null)),
false,
initialBody
).body()
assertNotNull(addNoteResult)
@@ -271,6 +272,7 @@ class GitLabApiTest : GitLabApiTestCase() {
volatileProjectMr1Iid,
GitLabDiffPositionInput("bd857928", "bd857928", 1, "063282e5", 1,
DiffPathsInputDTO("README.md", null)),
false,
initialBody
).body()
assertNotNull(addNoteResult)