From c4cf4f84496bf96bac752c90163a718d456917d8 Mon Sep 17 00:00:00 2001 From: Ivan Semenov Date: Sun, 8 Feb 2026 12:00:08 +0100 Subject: [PATCH] refactor [gitlab]: always load all MR discussions and events and don't await the full load on refresh We don't need to wait for the full discussions refresh on every MR operation, as it takes considerable time GitOrigin-RevId: 8d724e8dc462c38ad3c56828b739023af3ee12cd --- .../mergerequest/data/GitLabMergeRequest.kt | 69 +++++++---------- .../GitLabMergeRequestDiscussionsContainer.kt | 75 +++++++++---------- 2 files changed, 63 insertions(+), 81 deletions(-) diff --git a/plugins/gitlab/gitlab-core/src/org/jetbrains/plugins/gitlab/mergerequest/data/GitLabMergeRequest.kt b/plugins/gitlab/gitlab-core/src/org/jetbrains/plugins/gitlab/mergerequest/data/GitLabMergeRequest.kt index ad0129294f3a..be0c95fde702 100644 --- a/plugins/gitlab/gitlab-core/src/org/jetbrains/plugins/gitlab/mergerequest/data/GitLabMergeRequest.kt +++ b/plugins/gitlab/gitlab-core/src/org/jetbrains/plugins/gitlab/mergerequest/data/GitLabMergeRequest.kt @@ -170,7 +170,7 @@ internal class LoadedGitLabMergeRequest( .mapScoped { details -> GitLabMergeRequestChangesImpl(this, api, glMetadata, projectMapping, details) } .modelFlow(cs, LOG) - private val stateEventsHolder = + override val stateEvents by lazy { startGitLabRestETagListLoaderIn( cs, getMergeRequestStateEventsUri(glProject, iid), @@ -178,17 +178,15 @@ internal class LoadedGitLabMergeRequest( requestReloadFlow = mergeRequestReloadRequest.withInitial(Unit), requestRefreshFlow = mergeRequestRefreshRequest.combine(stateEventsRefreshRequest.withInitial(Unit)) { _, _ -> }, - - shouldTryToLoadAll = false + shouldTryToLoadAll = true ) { uri, eTag -> - api.rest.loadUpdatableJsonList( - GitLabApiRequestName.REST_GET_MERGE_REQUEST_STATE_EVENTS, uri, eTag - ) - } - override val stateEvents = - stateEventsHolder.resultOrErrorFlow.modelFlow(cs, LOG) + api.rest.loadUpdatableJsonList( + GitLabApiRequestName.REST_GET_MERGE_REQUEST_STATE_EVENTS, uri, eTag + ) + }.resultOrErrorFlow.modelFlow(cs, LOG) + } - private val labelEventsHolder = + override val labelEvents by lazy { startGitLabRestETagListLoaderIn( cs, getMergeRequestLabelEventsUri(glProject, iid), @@ -196,17 +194,15 @@ internal class LoadedGitLabMergeRequest( requestReloadFlow = mergeRequestReloadRequest.withInitial(Unit), requestRefreshFlow = mergeRequestRefreshRequest, - - shouldTryToLoadAll = false + shouldTryToLoadAll = true ) { uri, eTag -> - api.rest.loadUpdatableJsonList( - GitLabApiRequestName.REST_GET_MERGE_REQUEST_LABEL_EVENTS, uri, eTag - ) - } - override val labelEvents = - labelEventsHolder.resultOrErrorFlow.modelFlow(cs, LOG) + api.rest.loadUpdatableJsonList( + GitLabApiRequestName.REST_GET_MERGE_REQUEST_LABEL_EVENTS, uri, eTag + ) + }.resultOrErrorFlow.modelFlow(cs, LOG) + } - private val milestoneEventsHolder = + override val milestoneEvents by lazy { startGitLabRestETagListLoaderIn( cs, getMergeRequestMilestoneEventsUri(glProject, iid), @@ -214,15 +210,13 @@ internal class LoadedGitLabMergeRequest( requestReloadFlow = mergeRequestReloadRequest.withInitial(Unit), requestRefreshFlow = mergeRequestRefreshRequest, - - shouldTryToLoadAll = false + shouldTryToLoadAll = true ) { uri, eTag -> - api.rest.loadUpdatableJsonList( - GitLabApiRequestName.REST_GET_MERGE_REQUEST_MILESTONE_EVENTS, uri, eTag - ) - } - override val milestoneEvents = - milestoneEventsHolder.resultOrErrorFlow.modelFlow(cs, LOG) + api.rest.loadUpdatableJsonList( + GitLabApiRequestName.REST_GET_MERGE_REQUEST_MILESTONE_EVENTS, uri, eTag + ) + }.resultOrErrorFlow.modelFlow(cs, LOG) + } private val _isLoading: MutableStateFlow = MutableStateFlow(false) override val isLoading: SharedFlow = _isLoading.asSharedFlow() @@ -294,7 +288,6 @@ internal class LoadedGitLabMergeRequest( override fun refreshData() { cs.launch { updateData() - startRefreshCycle() } } @@ -306,20 +299,14 @@ internal class LoadedGitLabMergeRequest( private suspend fun updateData() { mergeRequestRefreshRequest.emit(Unit) - discussionsContainer.checkUpdates() - } - - private suspend fun startRefreshCycle() { - stateEventsHolder.loadAll() - labelEventsHolder.loadAll() - milestoneEventsHolder.loadAll() + discussionsContainer.requestDiscussionsRefresh() } override suspend fun merge(commitMessage: String) { withContext(cs.coroutineContext + Dispatchers.IO) { runMerge(commitMessage, withSquash = false) } - discussionsContainer.checkUpdates() + discussionsContainer.requestDiscussionsRefresh() GitLabStatistics.logMrActionExecuted(project, GitLabStatistics.MergeRequestAction.MERGE) } @@ -327,7 +314,7 @@ internal class LoadedGitLabMergeRequest( withContext(cs.coroutineContext + Dispatchers.IO) { runMerge(commitMessage, withSquash = true) } - discussionsContainer.checkUpdates() + discussionsContainer.requestDiscussionsRefresh() GitLabStatistics.logMrActionExecuted(project, GitLabStatistics.MergeRequestAction.SQUASH_MERGE) } @@ -335,7 +322,7 @@ internal class LoadedGitLabMergeRequest( withContext(cs.coroutineContext + Dispatchers.IO) { runRebase() } - discussionsContainer.checkUpdates() + discussionsContainer.requestDiscussionsRefresh() GitLabStatistics.logMrActionExecuted(project, GitLabStatistics.MergeRequestAction.REBASE) } @@ -389,7 +376,7 @@ internal class LoadedGitLabMergeRequest( .getResultOrThrow() updateMergeRequestData(updatedMergeRequest) } - discussionsContainer.checkUpdates() + discussionsContainer.requestDiscussionsRefresh() GitLabStatistics.logMrActionExecuted(project, GitLabStatistics.MergeRequestAction.POST_REVIEW) } @@ -405,7 +392,7 @@ internal class LoadedGitLabMergeRequest( updateMergeRequestData(updatedMergeRequest) } - discussionsContainer.checkUpdates() + discussionsContainer.requestDiscussionsRefresh() GitLabStatistics.logMrActionExecuted(project, GitLabStatistics.MergeRequestAction.SET_REVIEWERS) } @@ -417,7 +404,7 @@ internal class LoadedGitLabMergeRequest( updateMergeRequestData(updatedMergeRequest) } } - discussionsContainer.checkUpdates() + discussionsContainer.requestDiscussionsRefresh() GitLabStatistics.logMrActionExecuted(project, GitLabStatistics.MergeRequestAction.REVIEWER_REREVIEW) } diff --git a/plugins/gitlab/gitlab-core/src/org/jetbrains/plugins/gitlab/mergerequest/data/GitLabMergeRequestDiscussionsContainer.kt b/plugins/gitlab/gitlab-core/src/org/jetbrains/plugins/gitlab/mergerequest/data/GitLabMergeRequestDiscussionsContainer.kt index 4af9ea3f2849..d8e99893a90d 100644 --- a/plugins/gitlab/gitlab-core/src/org/jetbrains/plugins/gitlab/mergerequest/data/GitLabMergeRequestDiscussionsContainer.kt +++ b/plugins/gitlab/gitlab-core/src/org/jetbrains/plugins/gitlab/mergerequest/data/GitLabMergeRequestDiscussionsContainer.kt @@ -89,33 +89,31 @@ class GitLabMergeRequestDiscussionsContainerImpl( private val reloadRequests = MutableSharedFlow(replay = 1, onBufferOverflow = BufferOverflow.DROP_OLDEST).apply { tryEmit(Unit) } - private val updateRequests = MutableSharedFlow(replay = 1, onBufferOverflow = BufferOverflow.DROP_OLDEST) + private val refreshRequests = MutableSharedFlow(replay = 1, onBufferOverflow = BufferOverflow.DROP_OLDEST) private val discussionEvents = MutableSharedFlow>() - private val discussionsDataHolder = + private val nonEmptyDiscussionsData: SharedFlow>> by lazy { startGitLabRestETagListLoaderIn( cs, getMergeRequestDiscussionsUri(glProject, mr.iid), { it.id }, requestReloadFlow = reloadRequests, - requestRefreshFlow = updateRequests, + requestRefreshFlow = refreshRequests, requestChangeFlow = discussionEvents, - shouldTryToLoadAll = false + shouldTryToLoadAll = true ) { uri, eTag -> api.rest.loadUpdatableJsonList( GitLabApiRequestName.REST_GET_MERGE_REQUEST_DISCUSSIONS, uri, eTag ) - } - - private val nonEmptyDiscussionsData: SharedFlow>> = - discussionsDataHolder.resultOrErrorFlow + }.resultOrErrorFlow .mapCatching { discussions -> discussions.filter { it.notes.isNotEmpty() } } .modelFlow(cs, LOG) + } - override val discussions: Flow>> = + override val discussions: Flow>> by lazy { nonEmptyDiscussionsData .transformConsecutiveSuccesses { mapFiltered { !it.notes.first().system } @@ -131,8 +129,9 @@ class GitLabMergeRequestDiscussionsContainerImpl( ) } .modelFlow(cs, LOG) + } - override val systemNotes: Flow>> = + override val systemNotes: Flow>> by lazy { nonEmptyDiscussionsData .transformConsecutiveSuccesses { // When one note in a discussion is a system note, all are, so we check the first. @@ -145,12 +144,13 @@ class GitLabMergeRequestDiscussionsContainerImpl( ) } .modelFlow(cs, LOG) + } private val draftNotesEvents = MutableSharedFlow>() - private val draftNotesDataHolder = + private val draftNotesData by lazy { if (glMetadata == null || glMetadata.version < GitLabVersion(15, 9)) { - null + flowOf(Result.success(emptyList())) } else { startGitLabRestETagListLoaderIn( @@ -159,38 +159,36 @@ class GitLabMergeRequestDiscussionsContainerImpl( { it.id }, requestReloadFlow = reloadRequests, - requestRefreshFlow = updateRequests, + requestRefreshFlow = refreshRequests, requestChangeFlow = draftNotesEvents, - shouldTryToLoadAll = false + shouldTryToLoadAll = true ) { uri, eTag -> api.rest.loadUpdatableJsonList( GitLabApiRequestName.REST_GET_DRAFT_NOTES, uri, eTag ) - } - } - - private val draftNotesData = - (draftNotesDataHolder?.resultOrErrorFlow ?: flowOf(Result.success(emptyList()))) - .mapCatching { draftNotes -> + }.resultOrErrorFlow.mapCatching { draftNotes -> if (draftNotes.isEmpty()) return@mapCatching emptyList() draftNotes.map { it } - } - .modelFlow(cs, LOG) + }.modelFlow(cs, LOG) + } + } - override val draftNotes: Flow>> = flow { - draftNotesData - .transformConsecutiveSuccesses { - mapDataToModel( - GitLabMergeRequestDraftNoteRestDTO::id, - { - GitLabMergeRequestDraftNoteImpl(this, api, glMetadata, glProject, mr, { draftNotesEvents.emit(it) }, it, currentUser) - }, - { update(it) } - ) - }.collect(this) - }.modelFlow(cs, LOG) + override val draftNotes: Flow>> by lazy { + flow { + draftNotesData + .transformConsecutiveSuccesses { + mapDataToModel( + GitLabMergeRequestDraftNoteRestDTO::id, + { + GitLabMergeRequestDraftNoteImpl(this, api, glMetadata, glProject, mr, { draftNotesEvents.emit(it) }, it, currentUser) + }, + { update(it) } + ) + }.collect(this) + }.modelFlow(cs, LOG) + } private fun getDiscussionDraftNotes(discussionId: GitLabId): Flow>> { // Convert discussion ID down to REST ID as it's safer than converting from REST to GQL @@ -263,7 +261,7 @@ class GitLabMergeRequestDiscussionsContainerImpl( } withContext(NonCancellable) { draftNotesEvents.emit(AllDeleted()) - checkUpdates() + requestDiscussionsRefresh() } } GitLabStatistics.logMrActionExecuted(project, GitLabStatistics.MergeRequestAction.SUBMIT_DRAFT_NOTES) @@ -273,10 +271,7 @@ class GitLabMergeRequestDiscussionsContainerImpl( reloadRequests.emit(Unit) } - suspend fun checkUpdates() { - updateRequests.emit(Unit) - - draftNotesDataHolder?.loadAll() - discussionsDataHolder.loadAll() + suspend fun requestDiscussionsRefresh() { + refreshRequests.emit(Unit) } } \ No newline at end of file