RS comments fixes (#23118)

* Guard write/entry actions in the RS comments screens

A review of the wordpress-rs comments feature surfaced four defects sharing one
root cause: write and entry actions missing the in-flight / lifecycle guards the
like and reply paths already have.

- Detail: guard moderation against a double-tap that derives its target from the
  optimistic status, which fired two racing writes and could desync UI vs server.
- Edit: on a load error keep the progress spinner up instead of flashing an empty
  editable form (which also opened a save-clobber window).
- Edit: ignore back/discard while a save is in flight — the edit is already
  dispatched and would land regardless.
- List: bail out of the first-page fetch when no site is selected so the site
  getter can't throw while init races its async Finish.

* Suppress ReturnCount on moderateComment

The added in-flight guard gives moderateComment a third early return; suppress
ReturnCount to match the neighboring onLikeClicked / onReplyClicked handlers.

* Address review feedback on the RS comments guards

Follow-up to the write/entry-action guards, fixing gaps a reviewer found:

- List: move the no-selected-site guard from fetchFirstPage up to the initTab
  and refreshTab entry points. The old placement let initTab register a
  permanently-loading tab and let refreshTab reach clearPostTitles(site) — the
  same requireNotNull crash — before the deeper guard ran.
- Edit: pressing back while a save is in flight now surfaces a "saving" message
  instead of silently swallowing the press, so a stalled save no longer feels
  frozen.
- Edit: a failed comment load now records a terminal state in the retained ui
  state (rendered as an error, never the empty form) rather than delegating the
  close to a one-shot snackbar/action event, which could be dropped on a null
  context or an Activity recreation and strand the screen on a spinner.

* Tighten RS comments guards after self-review

- Add a test that a second moderation is allowed once the first completes,
  covering the in-flight flag's finally reset (a stuck flag would otherwise
  silently ignore all later moderations with green tests).
- Show the "saving" back-press message only once per in-flight save so
  repeated presses don't queue a stack of identical snackbars.
- Fix a now-stale doc comment: only the comment-detail load-error holder
  still relies on onDismissAction to close the screen.

* Guard the implicit approve-after-reply against racing moderations

approveAfterReply() called moderate(APPROVED) directly, bypassing the
isModerationInProgress guard, so an approve/spam/trash tapped during the
auto-approve's network call could fire a second, conflicting updateStatus and
desync the UI from the server. Hold the same guard across approveAfterReply's
moderate() call, and add a test that a moderation tapped during that window is
ignored.

Author: nbradbury

WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsViewModel.kt

@@ -100,6 +100,7 @@ class UnifiedCommentDetailsViewModel @Inject constructor(
     private var noteId: String? = null
     private var loadedComment: RsComment? = null
     private var isLikeInProgress = false
+    private var isModerationInProgress = false
 
     fun start(site: SiteModel, remoteCommentId: Long, noteId: String? = null) {
         if (isStarted) return
@@ -283,42 +284,62 @@ class UnifiedCommentDetailsViewModel @Inject constructor(
     }
 
     private suspend fun approveAfterReply() {
-        _uiState.value = _uiState.value?.copy(status = APPROVED)
-        val result = withContext(bgDispatcher) { moderate(APPROVED) }
-        if (result is RsResult.Error) {
-            _uiState.value = _uiState.value?.copy(status = UNAPPROVED)
-        } else {
-            // Match the legacy screen, which tracks the implicit approve when replying to an
-            // unapproved comment (this path is only reached from an unapproved comment).
-            trackCommentAction(Stat.COMMENT_APPROVED)
-            if (noteId != null) {
-                _commentModerated.value = Event(APPROVED)
+        // Hold the same guard moderateComment uses so this implicit approve can't race a user
+        // moderation: skip if one is already in flight, and keep the flag set across our own
+        // (multi-second) network call so an approve/spam/trash tapped during it is ignored rather
+        // than firing a second, conflicting updateStatus that would desync the UI and server.
+        if (isModerationInProgress) return
+        isModerationInProgress = true
+        try {
+            _uiState.value = _uiState.value?.copy(status = APPROVED)
+            val result = withContext(bgDispatcher) { moderate(APPROVED) }
+            if (result is RsResult.Error) {
+                _uiState.value = _uiState.value?.copy(status = UNAPPROVED)
+            } else {
+                // Match the legacy screen, which tracks the implicit approve when replying to an
+                // unapproved comment (this path is only reached from an unapproved comment).
+                trackCommentAction(Stat.COMMENT_APPROVED)
+                if (noteId != null) {
+                    _commentModerated.value = Event(APPROVED)
+                }
             }
+        } finally {
+            isModerationInProgress = false
         }
     }
 
+    @Suppress("ReturnCount")
     private fun moderateComment(newStatus: CommentStatus, closeOnSuccess: Boolean) {
         // The action footer stays visible while the comment loads, so ignore taps until then:
         // before the load completes the ui state holds a default status and the toggle handlers
         // would compute (and apply server-side) the wrong target status.
         if (loadedComment == null) return
         if (isOffline()) return
+        // Guard against a second moderation while one is in flight (fast double-tap): the target
+        // status is derived from the optimistic ui state, so racing requests could compute (and
+        // apply server-side) conflicting statuses and leave the UI and server out of sync.
+        if (isModerationInProgress) return
         val previousStatus = currentStatus()
+        isModerationInProgress = true
         launch {
-            _uiState.value = _uiState.value?.copy(status = newStatus)
-            val result = withContext(bgDispatcher) { moderate(newStatus) }
-            if (result is RsResult.Error) {
-                _uiState.value = _uiState.value?.copy(status = previousStatus)
-                showError(result.message, R.string.error_moderate_comment)
-            } else {
-                moderationStat(previousStatus, newStatus)?.let { trackCommentAction(it) }
-                _commentChanged.value = Event(Unit)
-                if (noteId != null) {
-                    _commentModerated.value = Event(newStatus)
-                }
-                if (closeOnSuccess) {
-                    _uiActionEvent.value = Event(Close)
+            try {
+                _uiState.value = _uiState.value?.copy(status = newStatus)
+                val result = withContext(bgDispatcher) { moderate(newStatus) }
+                if (result is RsResult.Error) {
+                    _uiState.value = _uiState.value?.copy(status = previousStatus)
+                    showError(result.message, R.string.error_moderate_comment)
+                } else {
+                    moderationStat(previousStatus, newStatus)?.let { trackCommentAction(it) }
+                    _commentChanged.value = Event(Unit)
+                    if (noteId != null) {
+                        _commentModerated.value = Event(newStatus)
+                    }
+                    if (closeOnSuccess) {
+                        _uiActionEvent.value = Event(Close)
+                    }
                 }
+            } finally {
+                isModerationInProgress = false
             }
         }
     }

WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentsEditViewModel.kt

@@ -71,6 +71,9 @@ class UnifiedCommentsEditViewModel @Inject constructor(
     // @Volatile covers the cross-thread clear.
     @Volatile
     private var isSaving = false
+    // Guards against queuing a stack of identical "saving" snackbars on repeated back-presses
+    // during a single in-flight save; reset when a new save starts.
+    private var savingBackNoticeShown = false
 
     private lateinit var site: SiteModel
 
@@ -86,6 +89,9 @@ class UnifiedCommentsEditViewModel @Inject constructor(
     data class EditCommentUiState(
         val canSaveChanges: Boolean = false,
         val showProgress: Boolean = false,
+        // A terminal "couldn't load the comment" state: the screen shows an error (not the form),
+        // held in retained state so a dropped snackbar/close event can't strand it on a spinner.
+        val loadFailed: Boolean = false,
         val progressText: UiString? = null,
         // Lives in ui state (not a one-shot event) so the dialog survives configuration changes,
         // like the DialogFragment it replaced.
@@ -171,6 +177,7 @@ class UnifiedCommentsEditViewModel @Inject constructor(
         _uiState.value?.let { uiState ->
             val editedCommentEssentials = uiState.editedComment
             isSaving = true
+            savingBackNoticeShown = false
             launch(bgDispatcher) {
                 try {
                     setLoadingState(SAVING)
@@ -183,6 +190,17 @@ class UnifiedCommentsEditViewModel @Inject constructor(
     }
 
     fun onBackPressed() {
+        // A save already in flight can't be discarded — the edit has been dispatched and will land
+        // server-side regardless. Surface that instead of silently swallowing the press, so a save
+        // that stalls to a network timeout doesn't leave the screen feeling frozen with no feedback.
+        // Only once per save, so repeated presses don't queue a stack of identical snackbars.
+        if (isSaving) {
+            if (!savingBackNoticeShown) {
+                savingBackNoticeShown = true
+                _onSnackbarMessage.value = Event(SnackbarMessageHolder(UiStringRes(R.string.saving_changes)))
+            }
+            return
+        }
         _uiState.value?.let {
             if (it.editedComment.isNotEqualTo(it.originalComment)) {
                 _uiState.value = it.copy(showDiscardDialog = true)
@@ -216,14 +234,20 @@ class UnifiedCommentsEditViewModel @Inject constructor(
                         originalComment = commentEssentials,
                         editedComment = commentEssentials
                     )
+                delay(LOADING_DELAY_MS)
+                setLoadingState(NOT_VISIBLE)
             } else {
-                _onSnackbarMessage.value = Event(SnackbarMessageHolder(
-                    message = UiStringRes(R.string.error_load_comment),
-                    onDismissAction = { _uiActionEvent.value = Event(CLOSE) }
-                ))
+                // The comment couldn't be loaded. Record the failure in the retained ui state
+                // (not just a one-shot event): showSnackbar bails when the fragment has no context,
+                // and the snackbar/close Events don't survive an Activity recreation — either would
+                // otherwise strand the screen on a spinner forever. The screen renders a terminal
+                // error from this state (never the empty form), which the user backs out of normally.
+                _uiState.value = (_uiState.value ?: EditCommentUiState()).copy(
+                    showProgress = false,
+                    loadFailed = true
+                )
+                _onSnackbarMessage.value = Event(SnackbarMessageHolder(UiStringRes(R.string.error_load_comment)))
             }
-            delay(LOADING_DELAY_MS)
-            setLoadingState(NOT_VISIBLE)
         }
     }
 

WordPress/src/main/java/org/wordpress/android/ui/comments/unified/compose/UnifiedCommentEditScreen.kt

@@ -105,6 +105,14 @@ fun UnifiedCommentEditScreen(
                         )
                     }
                 }
+            } else if (uiState.loadFailed) {
+                // Terminal load-error state: show the reason instead of an empty, editable form.
+                Text(
+                    text = stringResource(R.string.error_load_comment),
+                    modifier = Modifier
+                        .align(Alignment.Center)
+                        .padding(16.dp)
+                )
             } else {
                 EditFields(uiState, onFieldChange, scrollState)
             }

WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt

@@ -216,7 +216,12 @@ class CommentsRsListViewModel @Inject constructor(
 
     /** Loads the first page for [tab] unless it's already initialized. */
     @MainThread
+    @Suppress("ReturnCount")
     fun initTab(tab: CommentsRsListTab) {
+        // No selected site: init already queued a toast + Finish. Bail before registering the tab
+        // so we neither strand a permanently-loading tab nor let a later refreshTab reach the
+        // site getter (requireNotNull) via clearPostTitles.
+        if (_site == null) return
         // updateTabUiState inserts the tab synchronously, so this also blocks re-entry while
         // the first fetch is in flight.
         if (_tabStates.value.containsKey(tab)) return
@@ -234,7 +239,11 @@ class CommentsRsListViewModel @Inject constructor(
      * keep the current list on screen until the new page arrives.
      */
     @MainThread
+    @Suppress("ReturnCount")
     fun refreshTab(tab: CommentsRsListTab, isUserRefresh: Boolean = false) {
+        // As in initTab: with no selected site the clearPostTitles(site) call below would hit the
+        // requireNotNull. Guard the entry point rather than deeper down.
+        if (_site == null) return
         if (!_tabStates.value.containsKey(tab)) return
         // A first page is already on its way (initial load or another refresh); a second fetch
         // would only duplicate the request and double-bump the page generation.
@@ -467,6 +476,7 @@ class CommentsRsListViewModel @Inject constructor(
     }
 
     private fun fetchFirstPage(tab: CommentsRsListTab, showErrorSnackbar: Boolean = true) {
+        // Callers (initTab, refreshTab) guard against a null site, so the site getter below is safe.
         firstPageJobs[tab] = viewModelScope.launch {
             val params = commentsRsDataSource.firstPageParams(
                 status = tab.queryStatus,

WordPress/src/main/java/org/wordpress/android/ui/compose/utils/SnackbarHostStateExtensions.kt

@@ -13,7 +13,7 @@ import org.wordpress.android.ui.utils.UiHelpers
  * Shows a [SnackbarMessageHolder] on a Compose [SnackbarHostState], bridging the View-world Snackbar
  * length constants and dismiss-event codes the holder carries. onDismissAction fires from a finally
  * so it still runs if the coroutine is cancelled (e.g. the view is torn down while the snackbar is
- * showing) — the load-error holder relies on this to close the screen.
+ * showing) — the comment-detail load-error holder relies on this to close the screen.
  *
  * Call from the caller's own coroutine scope, e.g.
  * `viewLifecycleOwner.lifecycleScope.launch { snackbarHostState.showMessage(holder, context, uiHelpers) }`.

WordPress/src/test/java/org/wordpress/android/ui/comments/viewmodels/UnifiedCommentDetailsViewModelTest.kt

@@ -279,6 +279,59 @@ class UnifiedCommentDetailsViewModelTest : BaseUnitTest() {
         verify(commentsStore, times(0)).likeComment(site, REMOTE_COMMENT_ID, null, false)
     }
 
+    @Test
+    fun `onApproveClicked ignores a second tap while a moderation is in flight`() = test {
+        whenever(commentsRsDataSource.updateStatus(eq(site), eq(REMOTE_COMMENT_ID), any()))
+            .doSuspendableAnswer {
+                delay(LOAD_DELAY_MS)
+                RsResult.Success
+            }
+        viewModel.start(site, REMOTE_COMMENT_ID)
+
+        viewModel.onApproveClicked()
+        viewModel.onApproveClicked()
+        advanceUntilIdle()
+
+        verify(commentsRsDataSource, times(1)).updateStatus(site, REMOTE_COMMENT_ID, UNAPPROVED)
+        verify(commentsRsDataSource, times(0)).updateStatus(site, REMOTE_COMMENT_ID, APPROVED)
+    }
+
+    @Test
+    fun `a moderation tapped during the auto-approve after a reply is ignored`() = test {
+        // Replying to an unapproved comment fires an implicit moderate(APPROVED). A moderation
+        // tapped while that (suspended) approve is in flight must not race a second write.
+        whenever(commentsRsDataSource.getComment(site, REMOTE_COMMENT_ID)).thenReturn(UNAPPROVED_RS_COMMENT)
+        whenever(commentsRsDataSource.updateStatus(eq(site), eq(REMOTE_COMMENT_ID), any()))
+            .doSuspendableAnswer {
+                delay(LOAD_DELAY_MS)
+                RsResult.Success
+            }
+        viewModel.start(site, REMOTE_COMMENT_ID)
+
+        viewModel.onReplyClicked("nice post")
+        // The reply has completed and the auto-approve's moderate(APPROVED) is now suspended.
+        viewModel.onApproveClicked()
+        advanceUntilIdle()
+
+        verify(commentsRsDataSource, times(1)).updateStatus(site, REMOTE_COMMENT_ID, APPROVED)
+        verify(commentsRsDataSource, times(0)).updateStatus(site, REMOTE_COMMENT_ID, UNAPPROVED)
+    }
+
+    @Test
+    fun `a second moderation is allowed once the first has completed`() = test {
+        // Guards against the in-flight flag getting stuck true (e.g. a dropped finally reset), which
+        // would silently ignore every later moderation. The first call completes before the second.
+        viewModel.start(site, REMOTE_COMMENT_ID)
+
+        viewModel.onApproveClicked()
+        advanceUntilIdle()
+        viewModel.onApproveClicked()
+        advanceUntilIdle()
+
+        verify(commentsRsDataSource).updateStatus(site, REMOTE_COMMENT_ID, UNAPPROVED)
+        verify(commentsRsDataSource).updateStatus(site, REMOTE_COMMENT_ID, APPROVED)
+    }
+
     @Test
     fun `onEditClicked emits launch edit event with site comment identifier`() = test {
         viewModel.start(site, REMOTE_COMMENT_ID)

WordPress/src/test/java/org/wordpress/android/ui/comments/viewmodels/UnifiedCommentsEditViewModelTest.kt

@@ -1,6 +1,7 @@
 package org.wordpress.android.ui.comments.viewmodels
 
 import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.delay
 import kotlinx.coroutines.test.advanceUntilIdle
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.Assert.assertEquals
@@ -9,6 +10,7 @@ import org.junit.Test
 import org.mockito.Mock
 import org.mockito.kotlin.any
 import org.mockito.kotlin.check
+import org.mockito.kotlin.doSuspendableAnswer
 import org.mockito.kotlin.eq
 import org.mockito.kotlin.mock
 import org.mockito.kotlin.never
@@ -172,22 +174,31 @@ class UnifiedCommentsEditViewModelTest : BaseUnitTest() {
     }
 
     @Test
-    fun `Should map CommentIdentifier to default CommentEssentials if CommentIdentifier comment not found`() = test {
+    fun `Should show a terminal load-failed state and not an empty form if comment not found`() = test {
         whenever(getCommentUseCase.execute(site, remoteCommentId))
             .thenReturn(null)
         viewModel.start(site, siteCommentIdentifier)
         advanceUntilIdle()
 
-        assertThat(uiState[1].editedComment).isEqualTo(CommentEssentials())
+        // On load failure the screen shows a terminal error (retained state), never the empty,
+        // editable form, and the spinner is cleared so it can't strand.
+        val last = uiState.last()
+        assertThat(last.loadFailed).isTrue
+        assertThat(last.showProgress).isFalse
+        assertThat(last.editedComment).isEqualTo(CommentEssentials())
+        assertThat(onSnackbarMessage.firstOrNull()).isNotNull
     }
 
     @Test
-    fun `Should map CommentIdentifier to default CommentEssentials if CommentIdentifier not handled`() = test {
+    fun `Should show a terminal load-failed state if CommentIdentifier not handled`() = test {
         // ReaderCommentIdentifier is not supported by this class yet
         viewModel.start(site, ReaderCommentIdentifier(0L, 0L, 0L))
         advanceUntilIdle()
 
-        assertThat(uiState[1].editedComment).isEqualTo(CommentEssentials())
+        val last = uiState.last()
+        assertThat(last.loadFailed).isTrue
+        assertThat(last.showProgress).isFalse
+        assertThat(onSnackbarMessage.firstOrNull()).isNotNull
     }
 
     @Test
@@ -421,6 +432,36 @@ class UnifiedCommentsEditViewModelTest : BaseUnitTest() {
         assertThat(uiActionEvent.firstOrNull()).isEqualTo(CLOSE)
     }
 
+    @Test
+    fun `onBackPressed is ignored while a save is in flight`() = test {
+        whenever(commentsStore.getCommentByLocalSiteAndRemoteId(site.id, remoteCommentId))
+            .thenReturn(listOf(COMMENT_ENTITY))
+        // A suspending save keeps isSaving true across the back press.
+        whenever(commentsStore.updateEditComment(eq(site), any())).doSuspendableAnswer {
+            delay(SAVE_DELAY_MS)
+            CommentsActionPayload(CommentsActionData(emptyList(), 0))
+        }
+        val emailFieldType: FieldType = mock()
+        whenever(emailFieldType.matches(USER_EMAIL)).thenReturn(true)
+        whenever(emailFieldType.isValid).thenReturn { true }
+
+        viewModel.start(site, siteCommentIdentifier)
+        // Make an edit so that, without the guard, back would open the discard dialog.
+        viewModel.onValidateField("edited user email", emailFieldType)
+        // The save is in flight (suspended), so isSaving is still true when back is pressed.
+        viewModel.onActionMenuClicked()
+        viewModel.onBackPressed()
+        viewModel.onBackPressed()
+
+        // Back is swallowed (no discard dialog, no close) but surfaces a "saving" message instead
+        // of silently doing nothing — and only once, so repeated presses don't queue duplicates.
+        assertThat(uiState.last().showDiscardDialog).isFalse
+        assertThat(uiActionEvent).doesNotContain(CLOSE)
+        assertThat(onSnackbarMessage.filter { it.message == UiStringRes(R.string.saving_changes) }).hasSize(1)
+
+        advanceUntilIdle()
+    }
+
     @Test
     fun `Should update notification entity on save if NotificationCommentIdentifier`() = test {
         whenever(commentsStore.getCommentByLocalSiteAndRemoteId(site.id, remoteCommentId))
@@ -619,6 +660,7 @@ class UnifiedCommentsEditViewModelTest : BaseUnitTest() {
     companion object {
         private const val LOCAL_SITE_ID = 123
         private const val REMOTE_SITE_ID = 456L
+        private const val SAVE_DELAY_MS = 1000L
 
         private val COMMENT_ENTITY = CommentEntity(
             id = 1000,