From 2fc6f16668224aa63243e5467f66a37b6d398aad Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Thu, 2 Jul 2026 12:22:09 -0400 Subject: [PATCH 01/20] Add wordpress-rs comments list (Phase 2a: core browsing) Add a Compose comments list backed by wordpress-rs, following the postsrs/pagesrs pattern. There's no rs mobile-cache support for comments yet, so each tab pages directly against /wp/v2/comments using the response's nextPageParams as the cursor. - 5 filter tabs (All/Pending/Approved/Spam/Trashed). The All and Approved tabs use CommentStatus.Custom("all"/"approve") because WP_Comment_Query doesn't recognise the "approved" value the rs enum serialises to - Rows show avatar, bold "author on post" title (post titles resolved via a batched sparse-field request), snippet, date, pending indicator - Pull-to-refresh, load-more, retry snackbars, shimmer/empty/error states - Tapping a row opens the rs comment detail; the list refreshes on return - Gated in ActivityLauncher.viewUnifiedComments by RS_UNIFIED_COMMENTS + site capability (WP.com REST or application password), falling back to the legacy list; the now-dead per-tap rs gate inside the legacy list fragment is removed Batch moderation and search follow in stacked PRs. Co-Authored-By: Claude Fable 5 --- WordPress/src/main/AndroidManifest.xml | 4 + .../android/ui/ActivityLauncher.java | 23 +- .../comments/unified/CommentsRsDataSource.kt | 126 ++++++++- .../unified/UnifiedCommentListFragment.kt | 33 +-- .../ui/commentsrs/CommentsRsListActivity.kt | 76 +++++ .../ui/commentsrs/CommentsRsListEvent.kt | 10 + .../ui/commentsrs/CommentsRsListTab.kt | 48 ++++ .../ui/commentsrs/CommentsRsListUiState.kt | 34 +++ .../ui/commentsrs/CommentsRsListViewModel.kt | 229 +++++++++++++++ .../commentsrs/screens/CommentsRsListItem.kt | 143 ++++++++++ .../screens/CommentsRsListScreen.kt | 122 ++++++++ .../screens/CommentsRsTabListScreen.kt | 224 +++++++++++++++ .../android/ui/main/BaseAppCompatActivity.kt | 2 + .../unified/CommentsRsListMappingTest.kt | 110 ++++++++ .../ui/commentsrs/CommentsRsListTabTest.kt | 26 ++ .../commentsrs/CommentsRsListViewModelTest.kt | 266 ++++++++++++++++++ 16 files changed, 1445 insertions(+), 31 deletions(-) create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListEvent.kt create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListTab.kt create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt create mode 100644 WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsListMappingTest.kt create mode 100644 WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListTabTest.kt create mode 100644 WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt diff --git a/WordPress/src/main/AndroidManifest.xml b/WordPress/src/main/AndroidManifest.xml index 72b6c62ba88c..712f239dc07c 100644 --- a/WordPress/src/main/AndroidManifest.xml +++ b/WordPress/src/main/AndroidManifest.xml @@ -313,6 +313,10 @@ android:theme="@style/WordPress.NoActionBar" android:launchMode="singleTop" android:label="@string/comments"/> + () + data class RsComment( val authorName: String, val authorAvatarUrl: String, @@ -38,12 +49,36 @@ class CommentsRsDataSource @Inject constructor( val status: CommentStatus ) + /** One comment list row, mapped from [CommentWithViewContext]. */ + data class RsCommentListItem( + val remoteCommentId: Long, + val authorName: String, + val authorAvatarUrl: String, + val dateGmt: Date, + val contentHtml: String, + val postId: Long, + val status: CommentStatus + ) + /** Result of a write request, carrying the server error message when one is available. */ sealed interface RsResult { object Success : RsResult data class Error(val message: String?) : RsResult } + /** Result of fetching one page of the comment list. */ + sealed interface RsCommentsPageResult { + data class Success( + val comments: List, + /** Ready-made params for the next page; null when this was the last page. */ + val nextPageParams: CommentListParams?, + /** Total matching comments from the X-WP-Total header, when the server provides it. */ + val total: Int? + ) : RsCommentsPageResult + + data class Error(val message: String?) : RsCommentsPageResult + } + suspend fun getComment(site: SiteModel, commentId: Long): RsComment? = safe(errorValue = null) { val client = wpApiClientProvider.getWpApiClient(site) when ( @@ -56,6 +91,71 @@ class CommentsRsDataSource @Inject constructor( } } + /** + * Fetches one page of the site's comments. Pass [firstPageParams] for the first page and the + * previous page's [RsCommentsPageResult.Success.nextPageParams] for subsequent pages. + */ + suspend fun fetchCommentsPage(site: SiteModel, params: CommentListParams): RsCommentsPageResult = + safe(errorValue = RsCommentsPageResult.Error(null)) { + val client = wpApiClientProvider.getWpApiClient(site) + when (val result = client.request { it.comments().listWithViewContext(params) }) { + is WpRequestResult.Success -> RsCommentsPageResult.Success( + comments = result.response.data.map { it.toRsCommentListItem() }, + nextPageParams = result.response.nextPageParams, + total = result.response.headerMap.wpTotal()?.toInt() + ) + is WpRequestResult.WpError -> RsCommentsPageResult.Error(result.errorMessage) + else -> RsCommentsPageResult.Error(null) + } + } + + fun firstPageParams(status: RsCommentStatus?, search: String? = null): CommentListParams = + CommentListParams( + perPage = COMMENTS_PAGE_SIZE, + search = search, + status = status, + orderby = WpApiParamCommentsOrderBy.DATE_GMT, + order = WpApiParamOrder.DESC + ) + + /** + * Fetches post titles for the given [postIds] in a single sparse-field network call, returning + * a map of post id to rendered title. Ids already cached skip the network round-trip. + * (List rows show "author commented on {post}", but the comment payload only has the post id.) + */ + suspend fun fetchPostTitles(site: SiteModel, postIds: List): Map { + val result = mutableMapOf() + val uncached = mutableListOf() + for (id in postIds.distinct()) { + val cached = postTitleCache[id] + if (cached != null) result[id] = cached else uncached.add(id) + } + if (uncached.isEmpty()) return result + + safe(errorValue = Unit) { + val client = wpApiClientProvider.getWpApiClient(site) + val response = client.request { + it.posts().filterListWithViewContext( + PostEndpointType.Posts, + PostListParams(include = uncached), + listOf(SparseAnyPostFieldWithViewContext.ID, SparseAnyPostFieldWithViewContext.TITLE) + ) + } + if (response is WpRequestResult.Success) { + for (post in response.response.data) { + val id = post.id ?: continue + val title = post.title?.rendered.orEmpty() + postTitleCache[id] = title + result[id] = title + } + } else { + val message = (response as? WpRequestResult.WpError<*>)?.errorMessage + AppLog.w(AppLog.T.COMMENTS, "fetchPostTitles failed: $message") + } + } + return result + } + suspend fun updateStatus(site: SiteModel, commentId: Long, status: CommentStatus): RsResult = write(site) { it.comments().update(commentId, CommentUpdateParams(status = status.toRsCommentStatus())) } @@ -93,8 +193,7 @@ class CommentsRsDataSource @Inject constructor( private fun CommentWithViewContext.toRsComment() = RsComment( authorName = authorName, - authorAvatarUrl = (authorAvatarUrls[UserAvatarSize.Size96] - ?: authorAvatarUrls.values.firstOrNull { !it.isNullOrEmpty() }).orEmpty(), + authorAvatarUrl = pickAvatarUrl(), // dateGmt is a UTC java.util.Date (an absolute instant), so relative-time formatting is // correct regardless of the site's timezone — unlike the offset-less local `date` field. dateGmt = dateGmt, @@ -103,8 +202,25 @@ class CommentsRsDataSource @Inject constructor( postId = post, status = status.toAppCommentStatus() ) + + companion object { + internal const val COMMENTS_PAGE_SIZE = 30u + } } +internal fun CommentWithViewContext.toRsCommentListItem() = CommentsRsDataSource.RsCommentListItem( + remoteCommentId = id, + authorName = authorName, + authorAvatarUrl = pickAvatarUrl(), + dateGmt = dateGmt, + contentHtml = content.rendered, + postId = post, + status = status.toAppCommentStatus() +) + +internal fun CommentWithViewContext.pickAvatarUrl(): String = + (authorAvatarUrls[UserAvatarSize.Size96] ?: authorAvatarUrls.values.firstOrNull { !it.isNullOrEmpty() }).orEmpty() + internal fun CommentStatus.toRsCommentStatus(): RsCommentStatus = when (this) { CommentStatus.APPROVED -> RsCommentStatus.Approved CommentStatus.UNAPPROVED -> RsCommentStatus.Hold diff --git a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentListFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentListFragment.kt index e433ac99dc3a..9893a0f21d35 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentListFragment.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentListFragment.kt @@ -15,7 +15,6 @@ import org.wordpress.android.R import org.wordpress.android.WordPress import org.wordpress.android.databinding.UnifiedCommentListFragmentBinding import org.wordpress.android.fluxc.model.CommentStatus -import org.wordpress.android.ui.ActivityLauncher import org.wordpress.android.ui.comments.unified.CommentDetailsActivityContract.CommentDetailsActivityRequest import org.wordpress.android.ui.comments.unified.CommentDetailsActivityContract.CommentDetailsActivityResponse import org.wordpress.android.ui.comments.unified.CommentListUiModelHelper.ActionModeUiModel @@ -25,8 +24,6 @@ import org.wordpress.android.ui.comments.unified.CommentListUiModelHelper.Commen import org.wordpress.android.ui.comments.unified.CommentListUiModelHelper.ConfirmationDialogUiModel import org.wordpress.android.ui.comments.unified.CommentListUiModelHelper.ConfirmationDialogUiModel.Visible import org.wordpress.android.ui.mysite.SelectedSiteRepository -import org.wordpress.android.ui.prefs.experimentalfeatures.ExperimentalFeatures -import org.wordpress.android.ui.prefs.experimentalfeatures.ExperimentalFeatures.Feature import org.wordpress.android.ui.utils.UiHelpers import org.wordpress.android.util.NetworkUtilsWrapper import org.wordpress.android.util.SnackbarItem @@ -54,9 +51,6 @@ class UnifiedCommentListFragment : Fragment(R.layout.unified_comment_list_fragme @Inject lateinit var networkUtilsWrapper: NetworkUtilsWrapper - @Inject - lateinit var experimentalFeatures: ExperimentalFeatures - private lateinit var viewModel: UnifiedCommentListViewModel private lateinit var activityViewModel: UnifiedCommentActivityViewModel private lateinit var adapter: UnifiedCommentListAdapter @@ -166,25 +160,16 @@ class UnifiedCommentListFragment : Fragment(R.layout.unified_comment_list_fragme private fun showCommentDetails(commentId: Long, commentStatus: CommentStatus) { currentSnackbar?.dismiss() - val site = selectedSiteRepository.getSelectedSite() - // The rs detail can only authenticate WP.com-accessed or application-password sites, so - // fall back to the legacy detail elsewhere (same gating as the rs pages/posts screens). - val siteSupportsRs = site != null && (site.isUsingWpComRestApi || site.hasApplicationPassword()) - if (siteSupportsRs && experimentalFeatures.isEnabled(Feature.RS_UNIFIED_COMMENTS)) { - ActivityLauncher.viewUnifiedCommentsDetails( - context, - site, - commentId + // This legacy list is only reached when the RS_UNIFIED_COMMENTS flag is off or the site + // can't use wordpress-rs (see ActivityLauncher.viewUnifiedComments), so it always pairs + // with the legacy detail; the rs list launches the rs detail itself. + commentDetails.launch( + CommentDetailsActivityRequest( + commentId, + commentStatus, + selectedSiteRepository.getSelectedSite()!! ) - } else { - commentDetails.launch( - CommentDetailsActivityRequest( - commentId, - commentStatus, - selectedSiteRepository.getSelectedSite()!! - ) - ) - } + ) } val commentDetails = registerForActivityResult( diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt new file mode 100644 index 000000000000..f2a27e564543 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt @@ -0,0 +1,76 @@ +package org.wordpress.android.ui.commentsrs + +import android.content.Context +import android.content.Intent +import android.os.Bundle +import androidx.activity.result.contract.ActivityResultContracts +import androidx.activity.viewModels +import androidx.compose.runtime.collectAsState +import androidx.compose.runtime.getValue +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.lifecycleScope +import androidx.lifecycle.repeatOnLifecycle +import dagger.hilt.android.AndroidEntryPoint +import kotlinx.coroutines.launch +import org.wordpress.android.ui.comments.unified.UnifiedCommentsDetailsActivity +import org.wordpress.android.ui.compose.theme.AppThemeM3 +import org.wordpress.android.ui.main.BaseAppCompatActivity +import org.wordpress.android.ui.commentsrs.screens.CommentsRsListScreen +import org.wordpress.android.util.ToastUtils +import org.wordpress.android.util.extensions.setContent + +@AndroidEntryPoint +class CommentsRsListActivity : BaseAppCompatActivity() { + private val viewModel: CommentsRsListViewModel by viewModels() + + // The detail can moderate, reply, edit or delete, and doesn't report a result — refresh + // unconditionally on return so the list reflects whatever happened there. + private val detailLauncher = registerForActivityResult( + ActivityResultContracts.StartActivityForResult() + ) { + viewModel.refreshAllTabs() + } + + override fun onCreate(savedInstanceState: Bundle?) { + super.onCreate(savedInstanceState) + + observeEvents() + + setContent { + val tabStates by viewModel.tabStates.collectAsState() + AppThemeM3 { + CommentsRsListScreen( + tabStates = tabStates, + snackbarMessages = viewModel.snackbarMessages, + onInitTab = viewModel::initTab, + onRefreshTab = { tab -> viewModel.refreshTab(tab, isUserRefresh = true) }, + onLoadMore = viewModel::loadMore, + onNavigateBack = { onBackPressedDispatcher.onBackPressed() }, + onCommentClick = { commentId, _ -> viewModel.onCommentClick(commentId) } + ) + } + } + } + + private fun observeEvents() { + lifecycleScope.launch { + repeatOnLifecycle(Lifecycle.State.STARTED) { + viewModel.events.collect { event -> handleEvent(event) } + } + } + } + + private fun handleEvent(event: CommentsRsListEvent) { + when (event) { + is CommentsRsListEvent.OpenCommentDetail -> detailLauncher.launch( + UnifiedCommentsDetailsActivity.createIntent(this, event.site, event.remoteCommentId) + ) + is CommentsRsListEvent.ShowToast -> ToastUtils.showToast(this, event.messageResId) + is CommentsRsListEvent.Finish -> finish() + } + } + + companion object { + fun createIntent(context: Context) = Intent(context, CommentsRsListActivity::class.java) + } +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListEvent.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListEvent.kt new file mode 100644 index 000000000000..9fa94130cac3 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListEvent.kt @@ -0,0 +1,10 @@ +package org.wordpress.android.ui.commentsrs + +import androidx.annotation.StringRes +import org.wordpress.android.fluxc.model.SiteModel + +sealed interface CommentsRsListEvent { + data class OpenCommentDetail(val site: SiteModel, val remoteCommentId: Long) : CommentsRsListEvent + data class ShowToast(@StringRes val messageResId: Int) : CommentsRsListEvent + object Finish : CommentsRsListEvent +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListTab.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListTab.kt new file mode 100644 index 000000000000..66978db5abef --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListTab.kt @@ -0,0 +1,48 @@ +package org.wordpress.android.ui.commentsrs + +import androidx.annotation.StringRes +import org.wordpress.android.R +import uniffi.wp_api.CommentStatus as RsCommentStatus + +/** + * Filter tabs for the rs comments list, matching the legacy unified list minus Unreplied + * (deferred; it needs client-side threading of ~100 comments). + * + * [queryStatus] is the `status` query param for `/wp/v2/comments`. Two tabs use + * [RsCommentStatus.Custom] because `WP_Comment_Query` only recognises the literal values + * `approve` and `all` — wordpress-rs serialises [RsCommentStatus.Approved] as `approved`, + * which WordPress core treats as an unknown status and returns nothing for (wordpress-rs + * `comments.rs` serialisation test asserts `status=approved`). `all` means approved+hold, + * matching the legacy ALL filter (APPROVED+UNAPPROVED). + */ +enum class CommentsRsListTab( + @StringRes val labelResId: Int, + @StringRes val emptyMessageResId: Int, + val queryStatus: RsCommentStatus +) { + ALL( + R.string.comment_status_all, + R.string.comments_empty_list, + RsCommentStatus.Custom("all") + ), + PENDING( + R.string.comment_status_unapproved, + R.string.comments_empty_list_filtered_pending, + RsCommentStatus.Hold + ), + APPROVED( + R.string.comment_status_approved, + R.string.comments_empty_list_filtered_approved, + RsCommentStatus.Custom("approve") + ), + SPAM( + R.string.comment_status_spam, + R.string.comments_empty_list_filtered_spam, + RsCommentStatus.Spam + ), + TRASHED( + R.string.comment_status_trash, + R.string.comments_empty_list_filtered_trashed, + RsCommentStatus.Trash + ) +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt new file mode 100644 index 000000000000..34ae3fb9bac9 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt @@ -0,0 +1,34 @@ +package org.wordpress.android.ui.commentsrs + +import org.wordpress.android.fluxc.model.CommentStatus + +data class CommentsRsSnackbarMessage( + val message: String, + val actionLabel: String? = null, + val onAction: (() -> Unit)? = null +) + +/** One comment row. */ +data class CommentRsUiModel( + val remoteCommentId: Long, + val authorName: String, + val avatarUrl: String, + val snippet: String, + val relativeDate: String, + val status: CommentStatus, + val postId: Long, + /** Resolved asynchronously in a batched request; null until then. */ + val postTitle: String? = null +) { + val isPending: Boolean get() = status == CommentStatus.UNAPPROVED +} + +data class CommentsTabUiState( + val comments: List = emptyList(), + val isLoading: Boolean = false, + val isRefreshing: Boolean = false, + val isLoadingMore: Boolean = false, + val canLoadMore: Boolean = false, + val error: String? = null, + val isAuthError: Boolean = false +) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt new file mode 100644 index 000000000000..4ccba38514c0 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt @@ -0,0 +1,229 @@ +package org.wordpress.android.ui.commentsrs + +import androidx.annotation.MainThread +import androidx.lifecycle.ViewModel +import androidx.lifecycle.viewModelScope +import dagger.hilt.android.lifecycle.HiltViewModel +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.Job +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.receiveAsFlow +import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext +import org.wordpress.android.R +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.modules.BG_THREAD +import org.wordpress.android.ui.comments.unified.CommentsRsDataSource +import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsCommentListItem +import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsCommentsPageResult +import org.wordpress.android.ui.mysite.SelectedSiteRepository +import org.wordpress.android.ui.postsrs.PostRsErrorUtils +import org.wordpress.android.util.DateTimeUtilsWrapper +import org.wordpress.android.util.HtmlUtils +import org.wordpress.android.util.NetworkUtilsWrapper +import org.wordpress.android.util.WPAvatarUtilsWrapper +import org.wordpress.android.viewmodel.ResourceProvider +import uniffi.wp_api.CommentListParams +import javax.inject.Inject +import javax.inject.Named + +/** + * ViewModel for the wordpress-rs comments list. Unlike the postsrs/pagesrs screens there is no + * rs cache support for comments yet, so each tab pages directly against `/wp/v2/comments`, + * keeping the response's `nextPageParams` as the pagination cursor. + */ +@HiltViewModel +class CommentsRsListViewModel @Inject constructor( + selectedSiteRepository: SelectedSiteRepository, + private val commentsRsDataSource: CommentsRsDataSource, + private val resourceProvider: ResourceProvider, + private val networkUtilsWrapper: NetworkUtilsWrapper, + private val dateTimeUtilsWrapper: DateTimeUtilsWrapper, + private val avatarUtilsWrapper: WPAvatarUtilsWrapper, + @Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher +) : ViewModel() { + private val _tabStates = MutableStateFlow>(emptyMap()) + val tabStates: StateFlow> = _tabStates.asStateFlow() + + private val _events = Channel(Channel.BUFFERED) + val events = _events.receiveAsFlow() + + private val _snackbarMessages = Channel(Channel.BUFFERED) + val snackbarMessages = _snackbarMessages.receiveAsFlow() + + // Pagination cursors: the next-page params returned with each fetched page, per tab. + private val nextPageParams = mutableMapOf() + private val initializingTabs = mutableSetOf() + private val resolveTitleJobs = mutableMapOf() + + private val _site: SiteModel? = selectedSiteRepository.getSelectedSite() + private val site: SiteModel + get() = requireNotNull(_site) { "No selected site — Activity should have finished" } + + init { + if (_site == null) { + _events.trySend(CommentsRsListEvent.ShowToast(R.string.blog_not_found)) + _events.trySend(CommentsRsListEvent.Finish) + } + } + + /** Loads the first page for [tab] unless it's already initialized or initializing. */ + @MainThread + fun initTab(tab: CommentsRsListTab) { + if (_tabStates.value.containsKey(tab) || initializingTabs.contains(tab)) return + initializingTabs.add(tab) + updateTabUiState(tab) { copy(isLoading = true) } + fetchFirstPage(tab) { initializingTabs.remove(tab) } + } + + /** + * Re-fetches the first page for [tab]. The pull-to-refresh indicator is only shown for a + * user-initiated refresh; silent refreshes (returning from the detail) keep the current + * list on screen until the new page arrives. + */ + @MainThread + fun refreshTab(tab: CommentsRsListTab, isUserRefresh: Boolean = false) { + if (!_tabStates.value.containsKey(tab)) return + updateTabUiState(tab) { copy(isRefreshing = isUserRefresh, error = null) } + fetchFirstPage(tab) + } + + /** Refreshes every initialized tab; comments move between tabs when moderated. */ + @MainThread + fun refreshAllTabs() { + _tabStates.value.keys.forEach { refreshTab(it) } + } + + /** Loads the next page for [tab] if there is one and nothing else is in flight. */ + @MainThread + fun loadMore(tab: CommentsRsListTab) { + val current = getTabUiState(tab) + val params = nextPageParams[tab] + val isBusy = current.isLoading || current.isRefreshing || current.isLoadingMore + if (params == null || isBusy) return + + updateTabUiState(tab) { copy(isLoadingMore = true) } + viewModelScope.launch { + val result = withContext(bgDispatcher) { commentsRsDataSource.fetchCommentsPage(site, params) } + when (result) { + is RsCommentsPageResult.Success -> { + nextPageParams[tab] = result.nextPageParams + updateTabUiState(tab) { + copy( + // A comment can shift pages if the list changed server-side between + // requests, so dedupe on append. + comments = (comments + result.comments.map { it.toUiModel() }) + .distinctBy { it.remoteCommentId }, + isLoadingMore = false, + canLoadMore = result.nextPageParams != null + ) + } + resolvePostTitles(tab) + } + is RsCommentsPageResult.Error -> { + updateTabUiState(tab) { copy(isLoadingMore = false) } + _snackbarMessages.trySend(CommentsRsSnackbarMessage(errorMessage(result.message))) + } + } + } + } + + /** Opens the rs comment detail for the tapped row. */ + @MainThread + fun onCommentClick(remoteCommentId: Long) { + _events.trySend(CommentsRsListEvent.OpenCommentDetail(site, remoteCommentId)) + } + + private fun fetchFirstPage(tab: CommentsRsListTab, onComplete: () -> Unit = {}) { + viewModelScope.launch { + val params = commentsRsDataSource.firstPageParams(status = tab.queryStatus) + val result = withContext(bgDispatcher) { commentsRsDataSource.fetchCommentsPage(site, params) } + when (result) { + is RsCommentsPageResult.Success -> { + nextPageParams[tab] = result.nextPageParams + updateTabUiState(tab) { + copy( + comments = result.comments.map { it.toUiModel() }, + isLoading = false, + isRefreshing = false, + canLoadMore = result.nextPageParams != null, + error = null, + isAuthError = false + ) + } + resolvePostTitles(tab) + } + is RsCommentsPageResult.Error -> onFirstPageError(tab, result.message) + } + onComplete() + } + } + + /** + * First-page failure: when the tab already shows comments keep them and offer a retry + * snackbar; when it's empty, show the full-screen error state. + */ + private fun onFirstPageError(tab: CommentsRsListTab, message: String?) { + val friendly = errorMessage(message) + if (getTabUiState(tab).comments.isNotEmpty()) { + updateTabUiState(tab) { copy(isLoading = false, isRefreshing = false, error = null) } + _snackbarMessages.trySend( + CommentsRsSnackbarMessage( + message = friendly, + actionLabel = resourceProvider.getString(R.string.retry), + onAction = { refreshTab(tab) } + ) + ) + } else { + updateTabUiState(tab) { + copy(comments = emptyList(), isLoading = false, isRefreshing = false, error = friendly) + } + } + } + + /** + * Resolves the "commented on {post}" titles for rows that don't have one yet, in a single + * batched request per tab. Rows keep an author-only title until (or if ever) resolved. + */ + private fun resolvePostTitles(tab: CommentsRsListTab) { + val unresolvedIds = getTabUiState(tab).comments + .filter { it.postTitle == null && it.postId > 0 } + .map { it.postId } + .distinct() + if (unresolvedIds.isEmpty()) return + + resolveTitleJobs[tab]?.cancel() + resolveTitleJobs[tab] = viewModelScope.launch { + val titles = withContext(bgDispatcher) { commentsRsDataSource.fetchPostTitles(site, unresolvedIds) } + if (titles.isEmpty()) return@launch + updateTabUiState(tab) { + copy(comments = comments.map { it.copy(postTitle = titles[it.postId] ?: it.postTitle) }) + } + } + } + + /** The server message when it sent one, otherwise a friendly offline/generic message. */ + private fun errorMessage(serverMessage: String?): String = + serverMessage?.takeIf { it.isNotBlank() } + ?: PostRsErrorUtils.friendlyErrorMessage(null, null, resourceProvider, networkUtilsWrapper) + + private fun RsCommentListItem.toUiModel() = CommentRsUiModel( + remoteCommentId = remoteCommentId, + authorName = authorName.ifBlank { resourceProvider.getString(R.string.anonymous) }, + avatarUrl = avatarUtilsWrapper.rewriteAvatarUrlWithResource(authorAvatarUrl, R.dimen.avatar_sz_medium), + snippet = HtmlUtils.fastStripHtml(contentHtml).trim(), + relativeDate = dateTimeUtilsWrapper.javaDateToTimeSpan(dateGmt), + status = status, + postId = postId + ) + + private fun getTabUiState(tab: CommentsRsListTab): CommentsTabUiState = + _tabStates.value[tab] ?: CommentsTabUiState(isLoading = true) + + private fun updateTabUiState(tab: CommentsRsListTab, update: CommentsTabUiState.() -> CommentsTabUiState) { + _tabStates.value += (tab to getTabUiState(tab).update()) + } +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt new file mode 100644 index 000000000000..068421b3d396 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt @@ -0,0 +1,143 @@ +package org.wordpress.android.ui.commentsrs.screens + +import androidx.compose.foundation.background +import androidx.compose.foundation.clickable +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.IntrinsicSize +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxHeight +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.height +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.size +import androidx.compose.foundation.layout.width +import androidx.compose.foundation.shape.CircleShape +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.filled.Person +import androidx.compose.material3.HorizontalDivider +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Text +import androidx.compose.runtime.Composable +import androidx.compose.ui.Modifier +import androidx.compose.ui.draw.clip +import androidx.compose.ui.graphics.vector.rememberVectorPainter +import androidx.compose.ui.layout.ContentScale +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.text.AnnotatedString +import androidx.compose.ui.text.SpanStyle +import androidx.compose.ui.text.buildAnnotatedString +import androidx.compose.ui.text.font.FontWeight +import androidx.compose.ui.text.style.TextOverflow +import androidx.compose.ui.unit.dp +import coil.compose.AsyncImage +import org.wordpress.android.R +import org.wordpress.android.ui.commentsrs.CommentRsUiModel + +private val AVATAR_SIZE = 40.dp +private val PENDING_INDICATOR_WIDTH = 4.dp + +@Composable +fun CommentsRsListItem( + comment: CommentRsUiModel, + onClick: () -> Unit, + modifier: Modifier = Modifier +) { + Column( + modifier = modifier + .fillMaxWidth() + .background(MaterialTheme.colorScheme.surface) + .clickable(onClick = onClick) + ) { + Row(modifier = Modifier.height(IntrinsicSize.Min)) { + // The pending indicator: a colored bar on the row's leading edge, like the legacy + // list's status indicator. + Box( + modifier = Modifier + .width(PENDING_INDICATOR_WIDTH) + .fillMaxHeight() + .background( + if (comment.isPending) { + MaterialTheme.colorScheme.tertiary + } else { + MaterialTheme.colorScheme.surface.copy(alpha = 0f) + } + ) + ) + Row(modifier = Modifier.padding(start = 12.dp, top = 16.dp, end = 16.dp, bottom = 16.dp)) { + CommentAvatar(comment = comment) + Column( + modifier = Modifier + .padding(start = 16.dp) + .weight(1f) + ) { + Text( + text = commentTitle(comment), + style = MaterialTheme.typography.bodyLarge, + maxLines = 1, + overflow = TextOverflow.Ellipsis + ) + if (comment.snippet.isNotBlank()) { + Text( + text = comment.snippet, + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + maxLines = 2, + overflow = TextOverflow.Ellipsis, + modifier = Modifier.padding(top = 2.dp) + ) + } + Text( + text = comment.relativeDate, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + modifier = Modifier.padding(top = 4.dp) + ) + } + } + } + HorizontalDivider(modifier = Modifier.padding(start = 72.dp)) + } +} + +@Composable +private fun CommentAvatar(comment: CommentRsUiModel) { + val fallback = rememberVectorPainter(Icons.Filled.Person) + AsyncImage( + model = comment.avatarUrl.ifBlank { null }, + contentDescription = null, + contentScale = ContentScale.Crop, + fallback = fallback, + error = fallback, + modifier = Modifier + .size(AVATAR_SIZE) + .clip(CircleShape) + ) +} + +/** + * The row title: "{author} on {post title}" via the shared [R.string.comment_title] template with + * both parts bold (like legacy `CommentListUiUtils.formatCommentTitle`), or just the bold author + * name while the post title is unresolved. + */ +@Composable +private fun commentTitle(comment: CommentRsUiModel): AnnotatedString { + val postTitle = comment.postTitle?.trim().orEmpty() + val formatted = if (postTitle.isEmpty()) { + comment.authorName + } else { + stringResource(R.string.comment_title, comment.authorName, postTitle) + } + return buildAnnotatedString { + append(formatted) + boldRange(formatted, comment.authorName) + if (postTitle.isNotEmpty()) boldRange(formatted, postTitle) + } +} + +private fun AnnotatedString.Builder.boldRange(formatted: String, part: String) { + val start = formatted.indexOf(part) + if (start >= 0) { + addStyle(SpanStyle(fontWeight = FontWeight.Bold), start, start + part.length) + } +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt new file mode 100644 index 000000000000..aeb7fe31e3f8 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt @@ -0,0 +1,122 @@ +package org.wordpress.android.ui.commentsrs.screens + +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.pager.HorizontalPager +import androidx.compose.foundation.pager.rememberPagerState +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.automirrored.filled.ArrowBack +import androidx.compose.material3.ExperimentalMaterial3Api +import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.PrimaryScrollableTabRow +import androidx.compose.material3.Scaffold +import androidx.compose.material3.SnackbarHost +import androidx.compose.material3.SnackbarHostState +import androidx.compose.material3.SnackbarResult +import androidx.compose.material3.Tab +import androidx.compose.material3.Text +import androidx.compose.material3.TopAppBar +import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.remember +import androidx.compose.runtime.rememberCoroutineScope +import androidx.compose.runtime.snapshotFlow +import androidx.compose.ui.Modifier +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.unit.dp +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.emptyFlow +import kotlinx.coroutines.launch +import org.wordpress.android.R +import org.wordpress.android.ui.commentsrs.CommentsRsListTab +import org.wordpress.android.ui.commentsrs.CommentsRsSnackbarMessage +import org.wordpress.android.ui.commentsrs.CommentsTabUiState + +@OptIn(ExperimentalMaterial3Api::class) +@Composable +fun CommentsRsListScreen( + tabStates: Map, + snackbarMessages: Flow = emptyFlow(), + onInitTab: (CommentsRsListTab) -> Unit, + onRefreshTab: (CommentsRsListTab) -> Unit, + onLoadMore: (CommentsRsListTab) -> Unit, + onNavigateBack: () -> Unit, + onCommentClick: (Long, CommentsRsListTab) -> Unit +) { + val tabs = CommentsRsListTab.entries + val pagerState = rememberPagerState(pageCount = { tabs.size }) + val coroutineScope = rememberCoroutineScope() + val snackbarHostState = remember { SnackbarHostState() } + + LaunchedEffect(snackbarMessages) { + snackbarMessages.collect { msg -> + val result = snackbarHostState.showSnackbar( + message = msg.message, + actionLabel = msg.actionLabel + ) + if (result == SnackbarResult.ActionPerformed) { + msg.onAction?.invoke() + } + } + } + + Scaffold( + snackbarHost = { SnackbarHost(snackbarHostState) }, + topBar = { + TopAppBar( + title = { Text(text = stringResource(R.string.comments)) }, + navigationIcon = { + IconButton(onClick = onNavigateBack) { + Icon( + Icons.AutoMirrored.Filled.ArrowBack, + contentDescription = stringResource(R.string.back) + ) + } + } + ) + } + ) { contentPadding -> + Column(modifier = Modifier.fillMaxSize().padding(contentPadding)) { + PrimaryScrollableTabRow( + selectedTabIndex = pagerState.settledPage, + edgePadding = 0.dp + ) { + tabs.forEachIndexed { index, tab -> + Tab( + selected = pagerState.settledPage == index, + onClick = { + coroutineScope.launch { pagerState.animateScrollToPage(index) } + }, + unselectedContentColor = MaterialTheme.colorScheme.onSurfaceVariant, + text = { Text(text = stringResource(tab.labelResId)) } + ) + } + } + + LaunchedEffect(pagerState) { + snapshotFlow { pagerState.settledPage }.collect { page -> + onInitTab(tabs[page]) + } + } + + HorizontalPager( + state = pagerState, + modifier = Modifier.weight(1f) + ) { page -> + val tab = tabs[page] + val tabState = tabStates[tab] ?: CommentsTabUiState(isLoading = true) + + CommentsRsTabListScreen( + state = tabState, + emptyMessageResId = tab.emptyMessageResId, + onRefresh = { onRefreshTab(tab) }, + onLoadMore = { onLoadMore(tab) }, + onCommentClick = { commentId -> onCommentClick(commentId, tab) } + ) + } + } + } +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt new file mode 100644 index 000000000000..66d8bdf2ab7b --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt @@ -0,0 +1,224 @@ +package org.wordpress.android.ui.commentsrs.screens + +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.Spacer +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.height +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.size +import androidx.compose.foundation.lazy.LazyColumn +import androidx.compose.foundation.lazy.items +import androidx.compose.foundation.lazy.rememberLazyListState +import androidx.compose.foundation.rememberScrollState +import androidx.compose.foundation.shape.CircleShape +import androidx.compose.foundation.shape.RoundedCornerShape +import androidx.compose.foundation.verticalScroll +import androidx.compose.material3.Button +import androidx.compose.material3.CircularProgressIndicator +import androidx.compose.material3.ExperimentalMaterial3Api +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Text +import androidx.compose.material3.pulltorefresh.PullToRefreshBox +import androidx.compose.material3.pulltorefresh.PullToRefreshDefaults +import androidx.compose.material3.pulltorefresh.rememberPullToRefreshState +import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.snapshotFlow +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.draw.clip +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.text.style.TextAlign +import androidx.compose.ui.unit.dp +import kotlinx.coroutines.flow.distinctUntilChanged +import org.wordpress.android.R +import org.wordpress.android.ui.commentsrs.CommentRsUiModel +import org.wordpress.android.ui.commentsrs.CommentsTabUiState +import org.wordpress.android.ui.compose.components.ShimmerBox + +@OptIn(ExperimentalMaterial3Api::class) +@Composable +fun CommentsRsTabListScreen( + state: CommentsTabUiState, + emptyMessageResId: Int, + onRefresh: () -> Unit, + onLoadMore: () -> Unit, + onCommentClick: (Long) -> Unit, + modifier: Modifier = Modifier +) { + val pullToRefreshState = rememberPullToRefreshState() + + PullToRefreshBox( + modifier = modifier.fillMaxSize(), + isRefreshing = state.isRefreshing, + state = pullToRefreshState, + onRefresh = onRefresh, + indicator = { + PullToRefreshDefaults.Indicator( + state = pullToRefreshState, + isRefreshing = state.isRefreshing, + color = MaterialTheme.colorScheme.primary, + modifier = Modifier.align(Alignment.TopCenter) + ) + } + ) { + when { + state.isLoading -> ShimmerList() + state.error != null && state.comments.isEmpty() -> ErrorContent( + error = state.error, + onRetry = if (state.isAuthError) null else onRefresh + ) + state.comments.isEmpty() && !state.isRefreshing -> EmptyContent( + emptyMessageResId = emptyMessageResId + ) + else -> CommentListContent( + comments = state.comments, + isLoadingMore = state.isLoadingMore, + canLoadMore = state.canLoadMore, + onLoadMore = onLoadMore, + onCommentClick = onCommentClick + ) + } + } +} + +@Composable +private fun CommentListContent( + comments: List, + isLoadingMore: Boolean, + canLoadMore: Boolean, + onLoadMore: () -> Unit, + onCommentClick: (Long) -> Unit +) { + val listState = rememberLazyListState() + + LaunchedEffect(canLoadMore) { + if (!canLoadMore) return@LaunchedEffect + snapshotFlow { + val lastVisible = listState.layoutInfo.visibleItemsInfo.lastOrNull()?.index ?: 0 + val total = listState.layoutInfo.totalItemsCount + lastVisible >= total - LOAD_MORE_THRESHOLD + }.distinctUntilChanged().collect { shouldLoad -> + if (shouldLoad) onLoadMore() + } + } + + LazyColumn( + state = listState, + modifier = Modifier.fillMaxSize() + ) { + items( + items = comments, + key = { it.remoteCommentId } + ) { comment -> + CommentsRsListItem( + comment = comment, + onClick = { onCommentClick(comment.remoteCommentId) }, + modifier = Modifier.animateItem() + ) + } + + if (isLoadingMore) { + item(key = "loading_more") { + Box( + modifier = Modifier + .fillParentMaxWidth() + .padding(16.dp), + contentAlignment = Alignment.Center + ) { + CircularProgressIndicator( + modifier = Modifier.size(24.dp), + strokeWidth = 2.dp + ) + } + } + } + } +} + +@Composable +private fun ShimmerList() { + LazyColumn(modifier = Modifier.fillMaxSize()) { + items(SHIMMER_ITEM_COUNT) { + PlaceholderItem() + } + } +} + +@Composable +private fun PlaceholderItem() { + Row(modifier = Modifier.padding(16.dp)) { + ShimmerBox( + modifier = Modifier + .size(40.dp) + .clip(CircleShape) + ) + Column(modifier = Modifier.padding(start = 16.dp)) { + ShimmerBox( + modifier = Modifier + .size(width = 180.dp, height = 16.dp) + .clip(RoundedCornerShape(4.dp)) + ) + Spacer(modifier = Modifier.height(8.dp)) + ShimmerBox( + modifier = Modifier + .size(width = 260.dp, height = 14.dp) + .clip(RoundedCornerShape(4.dp)) + ) + } + } +} + +@Composable +private fun ErrorContent(error: String, onRetry: (() -> Unit)?) { + Column( + modifier = Modifier + .fillMaxSize() + .verticalScroll(rememberScrollState()) + .padding(32.dp), + horizontalAlignment = Alignment.CenterHorizontally, + verticalArrangement = Arrangement.Center + ) { + Text( + text = stringResource(R.string.error_generic), + style = MaterialTheme.typography.titleMedium + ) + Spacer(modifier = Modifier.height(8.dp)) + Text( + text = error, + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + textAlign = TextAlign.Center + ) + if (onRetry != null) { + Spacer(modifier = Modifier.height(16.dp)) + Button(onClick = onRetry) { + Text(text = stringResource(R.string.retry)) + } + } + } +} + +@Composable +private fun EmptyContent(emptyMessageResId: Int) { + Column( + modifier = Modifier + .fillMaxSize() + .verticalScroll(rememberScrollState()) + .padding(32.dp), + horizontalAlignment = Alignment.CenterHorizontally, + verticalArrangement = Arrangement.Center + ) { + Text( + text = stringResource(emptyMessageResId), + style = MaterialTheme.typography.bodyLarge, + color = MaterialTheme.colorScheme.onSurfaceVariant + ) + } +} + +private const val LOAD_MORE_THRESHOLD = 5 +private const val SHIMMER_ITEM_COUNT = 8 diff --git a/WordPress/src/main/java/org/wordpress/android/ui/main/BaseAppCompatActivity.kt b/WordPress/src/main/java/org/wordpress/android/ui/main/BaseAppCompatActivity.kt index 3b2bd1551ce3..6225fc81a75d 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/main/BaseAppCompatActivity.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/main/BaseAppCompatActivity.kt @@ -17,6 +17,7 @@ import org.wordpress.android.ui.jetpackoverlay.JetpackStaticPosterActivity import org.wordpress.android.ui.main.feedbackform.FeedbackFormActivity import org.wordpress.android.ui.media.MediaPreviewActivity import org.wordpress.android.ui.media.MediaSettingsActivity +import org.wordpress.android.ui.commentsrs.CommentsRsListActivity import org.wordpress.android.ui.mysite.menu.MenuActivity import org.wordpress.android.ui.mysite.personalization.PersonalizationActivity import org.wordpress.android.ui.navmenus.NavMenusActivity @@ -83,6 +84,7 @@ private fun isExcludedActivity(activity: BaseAppCompatActivity) = private val excludedActivities = listOf( BlazeCampaignParentActivity::class.java.name, BloggingPromptsListActivity::class.java.name, + CommentsRsListActivity::class.java.name, DebugSharedPreferenceFlagsActivity::class.java.name, DomainManagementActivity::class.java.name, EditJetpackSocialShareMessageActivity::class.java.name, diff --git a/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsListMappingTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsListMappingTest.kt new file mode 100644 index 000000000000..a3ff1a094ffc --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsListMappingTest.kt @@ -0,0 +1,110 @@ +package org.wordpress.android.ui.comments.unified + +import org.assertj.core.api.Assertions.assertThat +import org.junit.Test +import org.mockito.kotlin.mock +import org.wordpress.android.fluxc.model.CommentStatus.APPROVED +import org.wordpress.android.fluxc.model.CommentStatus.UNAPPROVED +import uniffi.wp_api.CommentContentWithViewContext +import uniffi.wp_api.CommentType +import uniffi.wp_api.CommentWithViewContext +import uniffi.wp_api.UserAvatarSize +import uniffi.wp_api.WpAdditionalFields +import uniffi.wp_api.WpApiParamCommentsOrderBy +import uniffi.wp_api.WpApiParamOrder +import java.util.Date +import uniffi.wp_api.CommentStatus as RsCommentStatus + +class CommentsRsListMappingTest { + @Test + fun `toRsCommentListItem maps the fields the list rows need`() { + val item = rsComment().toRsCommentListItem() + + assertThat(item.remoteCommentId).isEqualTo(COMMENT_ID) + assertThat(item.authorName).isEqualTo("Jane") + assertThat(item.authorAvatarUrl).isEqualTo("https://example.com/avatar96.png") + assertThat(item.dateGmt).isEqualTo(DATE_GMT) + assertThat(item.contentHtml).isEqualTo("

hello

") + assertThat(item.postId).isEqualTo(POST_ID) + assertThat(item.status).isEqualTo(APPROVED) + } + + @Test + fun `toRsCommentListItem maps hold status to unapproved`() { + val item = rsComment(status = RsCommentStatus.Hold).toRsCommentListItem() + + assertThat(item.status).isEqualTo(UNAPPROVED) + } + + @Test + fun `pickAvatarUrl prefers size 96`() { + val comment = rsComment( + avatarUrls = mapOf( + UserAvatarSize.Size24 to "https://example.com/avatar24.png", + UserAvatarSize.Size96 to "https://example.com/avatar96.png" + ) + ) + + assertThat(comment.pickAvatarUrl()).isEqualTo("https://example.com/avatar96.png") + } + + @Test + fun `pickAvatarUrl falls back to the first non-empty url`() { + val comment = rsComment( + avatarUrls = mapOf( + UserAvatarSize.Size24 to "", + UserAvatarSize.Size48 to "https://example.com/avatar48.png" + ) + ) + + assertThat(comment.pickAvatarUrl()).isEqualTo("https://example.com/avatar48.png") + } + + @Test + fun `pickAvatarUrl returns empty when there are no avatar urls`() { + assertThat(rsComment(avatarUrls = emptyMap()).pickAvatarUrl()).isEmpty() + } + + @Test + fun `firstPageParams requests newest comments with the given status and search`() { + val dataSource = CommentsRsDataSource(mock()) + + val params = dataSource.firstPageParams(RsCommentStatus.Spam, search = "query") + + assertThat(params.perPage).isEqualTo(CommentsRsDataSource.COMMENTS_PAGE_SIZE) + assertThat(params.status).isEqualTo(RsCommentStatus.Spam) + assertThat(params.search).isEqualTo("query") + assertThat(params.orderby).isEqualTo(WpApiParamCommentsOrderBy.DATE_GMT) + assertThat(params.order).isEqualTo(WpApiParamOrder.DESC) + } + + private fun rsComment( + status: RsCommentStatus = RsCommentStatus.Approved, + avatarUrls: Map = mapOf( + UserAvatarSize.Size96 to "https://example.com/avatar96.png" + ) + ) = CommentWithViewContext( + id = COMMENT_ID, + author = 7L, + authorName = "Jane", + authorUrl = "https://example.com", + content = CommentContentWithViewContext(rendered = "

hello

"), + date = "2026-07-01T12:00:00", + dateGmt = DATE_GMT, + link = "https://example.com/post/#comment-42", + parent = 0L, + post = POST_ID, + status = status, + commentType = CommentType.Comment, + authorAvatarUrls = avatarUrls, + // Mocked: the real WpAdditionalFields constructor loads the uniffi native library, + // which isn't available in local unit tests. + additionalFields = mock() + ) + + companion object { + private const val COMMENT_ID = 42L + private const val POST_ID = 99L + private val DATE_GMT = Date(1_700_000_000_000) + } +} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListTabTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListTabTest.kt new file mode 100644 index 000000000000..023bff617408 --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListTabTest.kt @@ -0,0 +1,26 @@ +package org.wordpress.android.ui.commentsrs + +import org.assertj.core.api.Assertions.assertThat +import org.junit.Test +import uniffi.wp_api.CommentStatus as RsCommentStatus + +class CommentsRsListTabTest { + @Test + fun `all tab queries status all which the server treats as approved plus hold`() { + assertThat(CommentsRsListTab.ALL.queryStatus).isEqualTo(RsCommentStatus.Custom("all")) + } + + @Test + fun `approved tab queries the literal approve status`() { + // WP_Comment_Query only recognises "approve"; the RsCommentStatus.Approved enum + // serialises to "approved", which the server treats as unknown and returns nothing for. + assertThat(CommentsRsListTab.APPROVED.queryStatus).isEqualTo(RsCommentStatus.Custom("approve")) + } + + @Test + fun `remaining tabs use the built-in statuses`() { + assertThat(CommentsRsListTab.PENDING.queryStatus).isEqualTo(RsCommentStatus.Hold) + assertThat(CommentsRsListTab.SPAM.queryStatus).isEqualTo(RsCommentStatus.Spam) + assertThat(CommentsRsListTab.TRASHED.queryStatus).isEqualTo(RsCommentStatus.Trash) + } +} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt new file mode 100644 index 000000000000..48b627613ca0 --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt @@ -0,0 +1,266 @@ +package org.wordpress.android.ui.commentsrs + +import androidx.lifecycle.viewModelScope +import app.cash.turbine.test +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.cancel +import kotlinx.coroutines.test.StandardTestDispatcher +import org.assertj.core.api.Assertions.assertThat +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.kotlin.any +import org.mockito.kotlin.anyOrNull +import org.mockito.kotlin.eq +import org.mockito.kotlin.times +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import org.wordpress.android.BaseUnitTest +import org.wordpress.android.R +import org.wordpress.android.fluxc.model.CommentStatus.APPROVED +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.ui.comments.unified.CommentsRsDataSource +import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsCommentListItem +import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsCommentsPageResult +import org.wordpress.android.ui.mysite.SelectedSiteRepository +import org.wordpress.android.util.DateTimeUtilsWrapper +import org.wordpress.android.util.NetworkUtilsWrapper +import org.wordpress.android.util.WPAvatarUtilsWrapper +import org.wordpress.android.viewmodel.ResourceProvider +import uniffi.wp_api.CommentListParams +import java.util.Date + +@ExperimentalCoroutinesApi +class CommentsRsListViewModelTest : BaseUnitTest(StandardTestDispatcher()) { + @Mock lateinit var selectedSiteRepository: SelectedSiteRepository + @Mock lateinit var commentsRsDataSource: CommentsRsDataSource + @Mock lateinit var resourceProvider: ResourceProvider + @Mock lateinit var networkUtilsWrapper: NetworkUtilsWrapper + @Mock lateinit var dateTimeUtilsWrapper: DateTimeUtilsWrapper + @Mock lateinit var avatarUtilsWrapper: WPAvatarUtilsWrapper + + private lateinit var site: SiteModel + private var activeViewModel: CommentsRsListViewModel? = null + + @Before + fun setUp() = test { + site = SiteModel().apply { + id = 1 + siteId = 123L + } + whenever(selectedSiteRepository.getSelectedSite()).thenReturn(site) + whenever(resourceProvider.getString(any())).thenReturn("string") + whenever(dateTimeUtilsWrapper.javaDateToTimeSpan(any())).thenReturn("2 hours ago") + whenever(avatarUtilsWrapper.rewriteAvatarUrlWithResource(any(), any())).thenAnswer { it.arguments[0] } + whenever(commentsRsDataSource.firstPageParams(any(), anyOrNull())).thenReturn(FIRST_PAGE) + whenever(commentsRsDataSource.fetchPostTitles(any(), any())).thenReturn(emptyMap()) + } + + @After + fun tearDown() { + activeViewModel?.viewModelScope?.cancel() + activeViewModel = null + } + + private fun createViewModel() = CommentsRsListViewModel( + selectedSiteRepository = selectedSiteRepository, + commentsRsDataSource = commentsRsDataSource, + resourceProvider = resourceProvider, + networkUtilsWrapper = networkUtilsWrapper, + dateTimeUtilsWrapper = dateTimeUtilsWrapper, + avatarUtilsWrapper = avatarUtilsWrapper, + bgDispatcher = testDispatcher() + ).also { activeViewModel = it } + + private suspend fun givenPage( + comments: List, + nextPageParams: CommentListParams? = null + ) { + whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())) + .thenReturn(RsCommentsPageResult.Success(comments, nextPageParams, comments.size)) + } + + @Test + fun `when no site selected, emits ShowToast and Finish`() = test { + whenever(selectedSiteRepository.getSelectedSite()).thenReturn(null) + + val viewModel = createViewModel() + + viewModel.events.test { + val first = awaitItem() + assertThat(first).isInstanceOf(CommentsRsListEvent.ShowToast::class.java) + assertThat((first as CommentsRsListEvent.ShowToast).messageResId).isEqualTo(R.string.blog_not_found) + assertThat(awaitItem()).isEqualTo(CommentsRsListEvent.Finish) + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `initTab loads the first page and maps rows`() = test { + givenPage(listOf(rsItem(id = 1), rsItem(id = 2)), nextPageParams = NEXT_PAGE) + val viewModel = createViewModel() + + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + + val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) + assertThat(state.isLoading).isFalse() + assertThat(state.comments).hasSize(2) + assertThat(state.comments.first().remoteCommentId).isEqualTo(1) + assertThat(state.comments.first().authorName).isEqualTo("Jane") + assertThat(state.comments.first().snippet).isEqualTo("hello") + assertThat(state.canLoadMore).isTrue() + } + + @Test + fun `initTab passes the tab's query status to the data source`() = test { + givenPage(emptyList()) + val viewModel = createViewModel() + + viewModel.initTab(CommentsRsListTab.APPROVED) + advanceUntilIdle() + + verify(commentsRsDataSource).firstPageParams(eq(CommentsRsListTab.APPROVED.queryStatus), anyOrNull()) + } + + @Test + fun `initTab is a no-op when the tab is already initialized`() = test { + givenPage(emptyList()) + val viewModel = createViewModel() + + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + + verify(commentsRsDataSource, times(1)).fetchCommentsPage(eq(site), any()) + } + + @Test + fun `initTab failure with no content shows the error state`() = test { + whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())) + .thenReturn(RsCommentsPageResult.Error("server said no")) + val viewModel = createViewModel() + + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + + val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) + assertThat(state.error).isEqualTo("server said no") + assertThat(state.isLoading).isFalse() + } + + @Test + fun `loadMore appends the next page and dedupes by comment id`() = test { + givenPage(listOf(rsItem(id = 1), rsItem(id = 2)), nextPageParams = NEXT_PAGE) + val viewModel = createViewModel() + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + givenPage(listOf(rsItem(id = 2), rsItem(id = 3)), nextPageParams = null) + + viewModel.loadMore(CommentsRsListTab.ALL) + advanceUntilIdle() + + val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) + assertThat(state.comments.map { it.remoteCommentId }).containsExactly(1L, 2L, 3L) + assertThat(state.canLoadMore).isFalse() + } + + @Test + fun `loadMore is a no-op when there is no next page`() = test { + givenPage(listOf(rsItem(id = 1)), nextPageParams = null) + val viewModel = createViewModel() + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + + viewModel.loadMore(CommentsRsListTab.ALL) + advanceUntilIdle() + + verify(commentsRsDataSource, times(1)).fetchCommentsPage(eq(site), any()) + } + + @Test + fun `refresh failure keeps the current comments and offers retry`() = test { + givenPage(listOf(rsItem(id = 1))) + val viewModel = createViewModel() + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())) + .thenReturn(RsCommentsPageResult.Error("boom")) + + viewModel.snackbarMessages.test { + viewModel.refreshTab(CommentsRsListTab.ALL, isUserRefresh = true) + advanceUntilIdle() + + val snackbar = awaitItem() + assertThat(snackbar.message).isEqualTo("boom") + assertThat(snackbar.onAction != null).isTrue() + cancelAndIgnoreRemainingEvents() + } + val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) + assertThat(state.comments).hasSize(1) + assertThat(state.error).isNull() + } + + @Test + fun `refreshAllTabs refreshes only initialized tabs`() = test { + givenPage(listOf(rsItem(id = 1))) + val viewModel = createViewModel() + viewModel.initTab(CommentsRsListTab.ALL) + viewModel.initTab(CommentsRsListTab.SPAM) + advanceUntilIdle() + + viewModel.refreshAllTabs() + advanceUntilIdle() + + // 2 init fetches + 2 refresh fetches, nothing for the 3 uninitialized tabs + verify(commentsRsDataSource, times(4)).fetchCommentsPage(eq(site), any()) + } + + @Test + fun `onCommentClick emits OpenCommentDetail`() = test { + givenPage(listOf(rsItem(id = 42))) + val viewModel = createViewModel() + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + + viewModel.events.test { + viewModel.onCommentClick(42L) + val event = awaitItem() + assertThat(event).isInstanceOf(CommentsRsListEvent.OpenCommentDetail::class.java) + assertThat((event as CommentsRsListEvent.OpenCommentDetail).remoteCommentId).isEqualTo(42L) + assertThat(event.site).isEqualTo(site) + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `post titles are resolved in a batch and applied to rows`() = test { + givenPage(listOf(rsItem(id = 1, postId = 10), rsItem(id = 2, postId = 20))) + whenever(commentsRsDataSource.fetchPostTitles(site, listOf(10L, 20L))) + .thenReturn(mapOf(10L to "First post", 20L to "Second post")) + val viewModel = createViewModel() + + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + + val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) + assertThat(state.comments.map { it.postTitle }).containsExactly("First post", "Second post") + } + + private fun rsItem(id: Long, postId: Long = 99L) = RsCommentListItem( + remoteCommentId = id, + authorName = "Jane", + authorAvatarUrl = "https://example.com/avatar.png", + dateGmt = Date(0), + contentHtml = "

hello

", + postId = postId, + status = APPROVED + ) + + companion object { + private val FIRST_PAGE = CommentListParams() + private val NEXT_PAGE = CommentListParams(page = 2u) + } +} From aad42b4227e9047840d617aa7e4c171e627e9e5c Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Thu, 2 Jul 2026 13:27:42 -0400 Subject: [PATCH 02/20] Rename RS comments experimental flag to cover the whole comments experience Co-Authored-By: Claude Opus 4.8 (1M context) --- WordPress/src/main/res/values/strings.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/src/main/res/values/strings.xml b/WordPress/src/main/res/values/strings.xml index 2cb702d1ac13..1778bc344c8d 100644 --- a/WordPress/src/main/res/values/strings.xml +++ b/WordPress/src/main/res/values/strings.xml @@ -1026,8 +1026,8 @@ You can turn off New Stats from the overflow menu in the Stats screen. New Pages List Show pages list in a more modern UI for WordPress.com and self-hosted sites with application passwords - RS Unified Comment Detail - Show the comment detail in a more modern UI for WordPress.com and self-hosted sites with application passwords + RS Unified Comments + Show comments in a more modern UI for WordPress.com and self-hosted sites with application passwords Failed to load post Failed to load page Copy URL From a2cc6b342ba9f9f5709d4e284b218f9400fe6ce1 Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Thu, 2 Jul 2026 13:54:58 -0400 Subject: [PATCH 03/20] Simplify rs comments list: drop dead/unused state, share page handling and SnackbarMessage Co-Authored-By: Claude Opus 4.8 (1M context) --- .../comments/unified/CommentsRsDataSource.kt | 7 +- .../ui/commentsrs/CommentsRsListUiState.kt | 9 +-- .../ui/commentsrs/CommentsRsListViewModel.kt | 67 ++++++++++--------- .../commentsrs/screens/CommentsRsListItem.kt | 3 +- .../screens/CommentsRsListScreen.kt | 4 +- .../screens/CommentsRsTabListScreen.kt | 12 ++-- .../commentsrs/CommentsRsListViewModelTest.kt | 2 +- 7 files changed, 49 insertions(+), 55 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt index 0abbc66bba71..a7714524a872 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt @@ -71,9 +71,7 @@ class CommentsRsDataSource @Inject constructor( data class Success( val comments: List, /** Ready-made params for the next page; null when this was the last page. */ - val nextPageParams: CommentListParams?, - /** Total matching comments from the X-WP-Total header, when the server provides it. */ - val total: Int? + val nextPageParams: CommentListParams? ) : RsCommentsPageResult data class Error(val message: String?) : RsCommentsPageResult @@ -101,8 +99,7 @@ class CommentsRsDataSource @Inject constructor( when (val result = client.request { it.comments().listWithViewContext(params) }) { is WpRequestResult.Success -> RsCommentsPageResult.Success( comments = result.response.data.map { it.toRsCommentListItem() }, - nextPageParams = result.response.nextPageParams, - total = result.response.headerMap.wpTotal()?.toInt() + nextPageParams = result.response.nextPageParams ) is WpRequestResult.WpError -> RsCommentsPageResult.Error(result.errorMessage) else -> RsCommentsPageResult.Error(null) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt index 34ae3fb9bac9..758c456cfd56 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt @@ -2,12 +2,6 @@ package org.wordpress.android.ui.commentsrs import org.wordpress.android.fluxc.model.CommentStatus -data class CommentsRsSnackbarMessage( - val message: String, - val actionLabel: String? = null, - val onAction: (() -> Unit)? = null -) - /** One comment row. */ data class CommentRsUiModel( val remoteCommentId: Long, @@ -29,6 +23,5 @@ data class CommentsTabUiState( val isRefreshing: Boolean = false, val isLoadingMore: Boolean = false, val canLoadMore: Boolean = false, - val error: String? = null, - val isAuthError: Boolean = false + val error: String? = null ) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt index 4ccba38514c0..32ad0d25dfad 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt @@ -21,6 +21,7 @@ import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsCommentL import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsCommentsPageResult import org.wordpress.android.ui.mysite.SelectedSiteRepository import org.wordpress.android.ui.postsrs.PostRsErrorUtils +import org.wordpress.android.ui.postsrs.SnackbarMessage import org.wordpress.android.util.DateTimeUtilsWrapper import org.wordpress.android.util.HtmlUtils import org.wordpress.android.util.NetworkUtilsWrapper @@ -51,7 +52,7 @@ class CommentsRsListViewModel @Inject constructor( private val _events = Channel(Channel.BUFFERED) val events = _events.receiveAsFlow() - private val _snackbarMessages = Channel(Channel.BUFFERED) + private val _snackbarMessages = Channel(Channel.BUFFERED) val snackbarMessages = _snackbarMessages.receiveAsFlow() // Pagination cursors: the next-page params returned with each fetched page, per tab. @@ -109,23 +110,10 @@ class CommentsRsListViewModel @Inject constructor( viewModelScope.launch { val result = withContext(bgDispatcher) { commentsRsDataSource.fetchCommentsPage(site, params) } when (result) { - is RsCommentsPageResult.Success -> { - nextPageParams[tab] = result.nextPageParams - updateTabUiState(tab) { - copy( - // A comment can shift pages if the list changed server-side between - // requests, so dedupe on append. - comments = (comments + result.comments.map { it.toUiModel() }) - .distinctBy { it.remoteCommentId }, - isLoadingMore = false, - canLoadMore = result.nextPageParams != null - ) - } - resolvePostTitles(tab) - } + is RsCommentsPageResult.Success -> applyPage(tab, result, append = true) is RsCommentsPageResult.Error -> { updateTabUiState(tab) { copy(isLoadingMore = false) } - _snackbarMessages.trySend(CommentsRsSnackbarMessage(errorMessage(result.message))) + _snackbarMessages.trySend(SnackbarMessage(errorMessage(result.message))) } } } @@ -142,26 +130,43 @@ class CommentsRsListViewModel @Inject constructor( val params = commentsRsDataSource.firstPageParams(status = tab.queryStatus) val result = withContext(bgDispatcher) { commentsRsDataSource.fetchCommentsPage(site, params) } when (result) { - is RsCommentsPageResult.Success -> { - nextPageParams[tab] = result.nextPageParams - updateTabUiState(tab) { - copy( - comments = result.comments.map { it.toUiModel() }, - isLoading = false, - isRefreshing = false, - canLoadMore = result.nextPageParams != null, - error = null, - isAuthError = false - ) - } - resolvePostTitles(tab) - } + is RsCommentsPageResult.Success -> applyPage(tab, result, append = false) is RsCommentsPageResult.Error -> onFirstPageError(tab, result.message) } onComplete() } } + /** + * A fetched page: stores the next-page cursor, applies the rows to the tab state, and kicks + * off post-title resolution. [append] is true when the page extends the list (load more) + * rather than replacing it (first page / refresh). + */ + private fun applyPage(tab: CommentsRsListTab, result: RsCommentsPageResult.Success, append: Boolean) { + nextPageParams[tab] = result.nextPageParams + val newRows = result.comments.map { it.toUiModel() } + updateTabUiState(tab) { + if (append) { + copy( + // A comment can shift pages if the list changed server-side between + // requests, so dedupe on append. + comments = (comments + newRows).distinctBy { it.remoteCommentId }, + isLoadingMore = false, + canLoadMore = result.nextPageParams != null + ) + } else { + copy( + comments = newRows, + isLoading = false, + isRefreshing = false, + canLoadMore = result.nextPageParams != null, + error = null + ) + } + } + resolvePostTitles(tab) + } + /** * First-page failure: when the tab already shows comments keep them and offer a retry * snackbar; when it's empty, show the full-screen error state. @@ -171,7 +176,7 @@ class CommentsRsListViewModel @Inject constructor( if (getTabUiState(tab).comments.isNotEmpty()) { updateTabUiState(tab) { copy(isLoading = false, isRefreshing = false, error = null) } _snackbarMessages.trySend( - CommentsRsSnackbarMessage( + SnackbarMessage( message = friendly, actionLabel = resourceProvider.getString(R.string.retry), onAction = { refreshTab(tab) } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt index 068421b3d396..860c8ebaf760 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt @@ -21,6 +21,7 @@ import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier import androidx.compose.ui.draw.clip +import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.vector.rememberVectorPainter import androidx.compose.ui.layout.ContentScale import androidx.compose.ui.res.stringResource @@ -60,7 +61,7 @@ fun CommentsRsListItem( if (comment.isPending) { MaterialTheme.colorScheme.tertiary } else { - MaterialTheme.colorScheme.surface.copy(alpha = 0f) + Color.Transparent } ) ) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt index aeb7fe31e3f8..ce6b05bc0c28 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt @@ -32,14 +32,14 @@ import kotlinx.coroutines.flow.emptyFlow import kotlinx.coroutines.launch import org.wordpress.android.R import org.wordpress.android.ui.commentsrs.CommentsRsListTab -import org.wordpress.android.ui.commentsrs.CommentsRsSnackbarMessage import org.wordpress.android.ui.commentsrs.CommentsTabUiState +import org.wordpress.android.ui.postsrs.SnackbarMessage @OptIn(ExperimentalMaterial3Api::class) @Composable fun CommentsRsListScreen( tabStates: Map, - snackbarMessages: Flow = emptyFlow(), + snackbarMessages: Flow = emptyFlow(), onInitTab: (CommentsRsListTab) -> Unit, onRefreshTab: (CommentsRsListTab) -> Unit, onLoadMore: (CommentsRsListTab) -> Unit, diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt index 66d8bdf2ab7b..89f118963254 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt @@ -69,7 +69,7 @@ fun CommentsRsTabListScreen( state.isLoading -> ShimmerList() state.error != null && state.comments.isEmpty() -> ErrorContent( error = state.error, - onRetry = if (state.isAuthError) null else onRefresh + onRetry = onRefresh ) state.comments.isEmpty() && !state.isRefreshing -> EmptyContent( emptyMessageResId = emptyMessageResId @@ -173,7 +173,7 @@ private fun PlaceholderItem() { } @Composable -private fun ErrorContent(error: String, onRetry: (() -> Unit)?) { +private fun ErrorContent(error: String, onRetry: () -> Unit) { Column( modifier = Modifier .fillMaxSize() @@ -193,11 +193,9 @@ private fun ErrorContent(error: String, onRetry: (() -> Unit)?) { color = MaterialTheme.colorScheme.onSurfaceVariant, textAlign = TextAlign.Center ) - if (onRetry != null) { - Spacer(modifier = Modifier.height(16.dp)) - Button(onClick = onRetry) { - Text(text = stringResource(R.string.retry)) - } + Spacer(modifier = Modifier.height(16.dp)) + Button(onClick = onRetry) { + Text(text = stringResource(R.string.retry)) } } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt index 48b627613ca0..59ec8c3694f5 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt @@ -78,7 +78,7 @@ class CommentsRsListViewModelTest : BaseUnitTest(StandardTestDispatcher()) { nextPageParams: CommentListParams? = null ) { whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())) - .thenReturn(RsCommentsPageResult.Success(comments, nextPageParams, comments.size)) + .thenReturn(RsCommentsPageResult.Success(comments, nextPageParams)) } @Test From 3cd35f7c257686d983b52601999238b881b85c21 Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Thu, 2 Jul 2026 14:37:40 -0400 Subject: [PATCH 04/20] Use new string keys for the renamed experimental flag copy Danger requires immutable string keys: changed copy gets a new key rather than editing the old one in place. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ui/prefs/experimentalfeatures/ExperimentalFeatures.kt | 4 ++-- WordPress/src/main/res/values/strings.xml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/prefs/experimentalfeatures/ExperimentalFeatures.kt b/WordPress/src/main/java/org/wordpress/android/ui/prefs/experimentalfeatures/ExperimentalFeatures.kt index c955267e611e..59abcf640bb2 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/prefs/experimentalfeatures/ExperimentalFeatures.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/prefs/experimentalfeatures/ExperimentalFeatures.kt @@ -47,8 +47,8 @@ class ExperimentalFeatures @Inject constructor( ), RS_UNIFIED_COMMENTS( "rs_unified_comments", - R.string.experimental_rs_unified_comments, - R.string.experimental_rs_unified_comments_description + R.string.experimental_rs_comments, + R.string.experimental_rs_comments_description ); } } diff --git a/WordPress/src/main/res/values/strings.xml b/WordPress/src/main/res/values/strings.xml index 1778bc344c8d..c200d82bd0a2 100644 --- a/WordPress/src/main/res/values/strings.xml +++ b/WordPress/src/main/res/values/strings.xml @@ -1026,8 +1026,8 @@ You can turn off New Stats from the overflow menu in the Stats screen. New Pages List Show pages list in a more modern UI for WordPress.com and self-hosted sites with application passwords - RS Unified Comments - Show comments in a more modern UI for WordPress.com and self-hosted sites with application passwords + RS Unified Comments + Show comments in a more modern UI for WordPress.com and self-hosted sites with application passwords Failed to load post Failed to load page Copy URL From b1bf05841d4eb61907ed49fd1a848f6eb064248a Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Thu, 2 Jul 2026 15:21:34 -0400 Subject: [PATCH 05/20] Harden rs comments list paging and post-title resolution - Key the post-title cache by (site, post id) so titles can't leak across sites, and negative-cache unresolvable ids so they aren't re-requested on every page load - Set perPage explicitly on title batches (server default of 10 silently truncated larger batches) and fall back to the pages endpoint for comments left unresolved by the posts endpoint - Discard load-more results whose fetch predates the latest applied first page, preventing stale pages/cursors after a silent refresh - Offer a retry action on load-more failures and auto-advance past pages that dedupe away entirely, so pagination can't stall at the list bottom Co-Authored-By: Claude Opus 4.8 (1M context) --- .../comments/unified/CommentsRsDataSource.kt | 74 ++++++++++++++----- .../ui/commentsrs/CommentsRsListViewModel.kt | 34 ++++++++- .../commentsrs/CommentsRsListViewModelTest.kt | 66 +++++++++++++++++ 3 files changed, 152 insertions(+), 22 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt index a7714524a872..44bebc60297c 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt @@ -37,7 +37,10 @@ import uniffi.wp_api.CommentStatus as RsCommentStatus class CommentsRsDataSource @Inject constructor( private val wpApiClientProvider: WpApiClientProvider ) { - private val postTitleCache = ConcurrentHashMap() + // Keyed by (local site id, post id): post ids are only unique per site, and this is a + // singleton shared across site switches. An empty value is a negative-cache entry for ids + // whose title can't be resolved (custom post types, non-published posts). + private val postTitleCache = ConcurrentHashMap, String>() data class RsComment( val authorName: String, @@ -116,43 +119,74 @@ class CommentsRsDataSource @Inject constructor( ) /** - * Fetches post titles for the given [postIds] in a single sparse-field network call, returning + * Fetches post titles for the given [postIds] in batched sparse-field network calls, returning * a map of post id to rendered title. Ids already cached skip the network round-trip. * (List rows show "author commented on {post}", but the comment payload only has the post id.) + * + * Comments can be attached to posts or pages, so unresolved ids fall back to the pages + * endpoint. Ids neither endpoint returns (custom post types, non-published posts) resolve to + * an empty title and are negative-cached so they aren't re-requested on every page load. */ suspend fun fetchPostTitles(site: SiteModel, postIds: List): Map { val result = mutableMapOf() val uncached = mutableListOf() for (id in postIds.distinct()) { - val cached = postTitleCache[id] + val cached = postTitleCache[site.id to id] if (cached != null) result[id] = cached else uncached.add(id) } if (uncached.isEmpty()) return result safe(errorValue = Unit) { - val client = wpApiClientProvider.getWpApiClient(site) - val response = client.request { - it.posts().filterListWithViewContext( - PostEndpointType.Posts, - PostListParams(include = uncached), - listOf(SparseAnyPostFieldWithViewContext.ID, SparseAnyPostFieldWithViewContext.TITLE) - ) - } - if (response is WpRequestResult.Success) { - for (post in response.response.data) { - val id = post.id ?: continue - val title = post.title?.rendered.orEmpty() - postTitleCache[id] = title - result[id] = title - } + val unresolved = fetchTitlesFromEndpoint(site, PostEndpointType.Posts, uncached, result) + ?: return@safe + val stillUnresolved = if (unresolved.isEmpty()) { + unresolved } else { - val message = (response as? WpRequestResult.WpError<*>)?.errorMessage - AppLog.w(AppLog.T.COMMENTS, "fetchPostTitles failed: $message") + fetchTitlesFromEndpoint(site, PostEndpointType.Pages, unresolved, result) ?: return@safe + } + // Only reached when both endpoints succeeded, so these ids genuinely have no + // resolvable title (as opposed to a transient request failure). + for (id in stillUnresolved) { + postTitleCache[site.id to id] = "" + result[id] = "" } } return result } + /** + * Fetches titles for [ids] from [endpointType] into [into] (and the cache), returning the ids + * the endpoint didn't include, or null when the request failed. [PostListParams.perPage] is + * set explicitly: the server default of 10 would silently truncate larger batches. + */ + private suspend fun fetchTitlesFromEndpoint( + site: SiteModel, + endpointType: PostEndpointType, + ids: List, + into: MutableMap + ): List? { + val client = wpApiClientProvider.getWpApiClient(site) + val response = client.request { + it.posts().filterListWithViewContext( + endpointType, + PostListParams(include = ids, perPage = ids.size.toUInt()), + listOf(SparseAnyPostFieldWithViewContext.ID, SparseAnyPostFieldWithViewContext.TITLE) + ) + } + if (response !is WpRequestResult.Success) { + val message = (response as? WpRequestResult.WpError<*>)?.errorMessage + AppLog.w(AppLog.T.COMMENTS, "fetchPostTitles ($endpointType) failed: $message") + return null + } + for (post in response.response.data) { + val id = post.id ?: continue + val title = post.title?.rendered.orEmpty() + postTitleCache[site.id to id] = title + into[id] = title + } + return ids.filter { it !in into } + } + suspend fun updateStatus(site: SiteModel, commentId: Long, status: CommentStatus): RsResult = write(site) { it.comments().update(commentId, CommentUpdateParams(status = status.toRsCommentStatus())) } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt index 32ad0d25dfad..3f9711f2b716 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt @@ -57,6 +57,10 @@ class CommentsRsListViewModel @Inject constructor( // Pagination cursors: the next-page params returned with each fetched page, per tab. private val nextPageParams = mutableMapOf() + + // Bumped whenever a first page is applied. A load-more result whose fetch started before the + // bump paged from a cursor that predates the new page 1, so it's stale and gets discarded. + private val pageGenerations = mutableMapOf() private val initializingTabs = mutableSetOf() private val resolveTitleJobs = mutableMapOf() @@ -107,13 +111,38 @@ class CommentsRsListViewModel @Inject constructor( if (params == null || isBusy) return updateTabUiState(tab) { copy(isLoadingMore = true) } + val generation = pageGenerations[tab] viewModelScope.launch { val result = withContext(bgDispatcher) { commentsRsDataSource.fetchCommentsPage(site, params) } + if (pageGenerations[tab] != generation) { + // A refresh replaced the list while this page was in flight, so its cursor is + // stale; appending it would mix old and new pages and corrupt the cursor chain. + updateTabUiState(tab) { copy(isLoadingMore = false) } + return@launch + } when (result) { - is RsCommentsPageResult.Success -> applyPage(tab, result, append = true) + is RsCommentsPageResult.Success -> { + val sizeBefore = getTabUiState(tab).comments.size + applyPage(tab, result, append = true) + // When the whole page deduped away (the list shifted server-side), the + // scroll position hasn't changed so the load-more trigger won't re-fire; + // advance to the next page directly. + if (result.comments.isNotEmpty() && + getTabUiState(tab).comments.size == sizeBefore && + nextPageParams[tab] != null + ) { + loadMore(tab) + } + } is RsCommentsPageResult.Error -> { updateTabUiState(tab) { copy(isLoadingMore = false) } - _snackbarMessages.trySend(SnackbarMessage(errorMessage(result.message))) + _snackbarMessages.trySend( + SnackbarMessage( + message = errorMessage(result.message), + actionLabel = resourceProvider.getString(R.string.retry), + onAction = { loadMore(tab) } + ) + ) } } } @@ -143,6 +172,7 @@ class CommentsRsListViewModel @Inject constructor( * rather than replacing it (first page / refresh). */ private fun applyPage(tab: CommentsRsListTab, result: RsCommentsPageResult.Success, append: Boolean) { + if (!append) pageGenerations[tab] = (pageGenerations[tab] ?: 0) + 1 nextPageParams[tab] = result.nextPageParams val newRows = result.comments.map { it.toUiModel() } updateTabUiState(tab) { diff --git a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt index 59ec8c3694f5..825934f3ac64 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt @@ -167,6 +167,71 @@ class CommentsRsListViewModelTest : BaseUnitTest(StandardTestDispatcher()) { assertThat(state.canLoadMore).isFalse() } + @Test + fun `stale loadMore result arriving after a silent refresh is discarded`() = test { + givenPage(listOf(rsItem(id = 1), rsItem(id = 2)), nextPageParams = NEXT_PAGE) + val viewModel = createViewModel() + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + // Next two fetches: the silent refresh gets a fresh first page; the concurrently + // launched loadMore gets the (now stale) old second page. + whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())).thenReturn( + RsCommentsPageResult.Success(listOf(rsItem(id = 10), rsItem(id = 11)), NEXT_PAGE), + RsCommentsPageResult.Success(listOf(rsItem(id = 3), rsItem(id = 4)), null) + ) + + viewModel.refreshTab(CommentsRsListTab.ALL) // silent: sets no busy flag + viewModel.loadMore(CommentsRsListTab.ALL) + advanceUntilIdle() + + val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) + assertThat(state.comments.map { it.remoteCommentId }).containsExactly(10L, 11L) + assertThat(state.isLoadingMore).isFalse() + assertThat(state.canLoadMore).isTrue() + } + + @Test + fun `loadMore failure offers a retry snackbar`() = test { + givenPage(listOf(rsItem(id = 1)), nextPageParams = NEXT_PAGE) + val viewModel = createViewModel() + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())) + .thenReturn(RsCommentsPageResult.Error("boom")) + + viewModel.snackbarMessages.test { + viewModel.loadMore(CommentsRsListTab.ALL) + advanceUntilIdle() + + val snackbar = awaitItem() + assertThat(snackbar.message).isEqualTo("boom") + assertThat(snackbar.onAction != null).isTrue() + cancelAndIgnoreRemainingEvents() + } + assertThat(viewModel.tabStates.value.getValue(CommentsRsListTab.ALL).isLoadingMore).isFalse() + } + + @Test + fun `fully deduplicated page auto-advances to the next page`() = test { + givenPage(listOf(rsItem(id = 1), rsItem(id = 2)), nextPageParams = NEXT_PAGE) + val viewModel = createViewModel() + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + // The next page duplicates the current rows entirely (server-side shift), then the + // page after that has the genuinely new comment. + whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())).thenReturn( + RsCommentsPageResult.Success(listOf(rsItem(id = 1), rsItem(id = 2)), THIRD_PAGE), + RsCommentsPageResult.Success(listOf(rsItem(id = 3)), null) + ) + + viewModel.loadMore(CommentsRsListTab.ALL) + advanceUntilIdle() + + val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) + assertThat(state.comments.map { it.remoteCommentId }).containsExactly(1L, 2L, 3L) + assertThat(state.canLoadMore).isFalse() + } + @Test fun `loadMore is a no-op when there is no next page`() = test { givenPage(listOf(rsItem(id = 1)), nextPageParams = null) @@ -262,5 +327,6 @@ class CommentsRsListViewModelTest : BaseUnitTest(StandardTestDispatcher()) { companion object { private val FIRST_PAGE = CommentListParams() private val NEXT_PAGE = CommentListParams(page = 2u) + private val THIRD_PAGE = CommentListParams(page = 3u) } } From 82663174031a9aa483b061bdb670e32a581171a1 Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Thu, 2 Jul 2026 15:43:17 -0400 Subject: [PATCH 06/20] Address review findings in rs comments paging hardening - Chunk post-title batches to the server's per_page maximum of 100; larger values are rejected outright with rest_invalid_param - Always attempt the pages endpoint for ids the posts endpoint didn't resolve, and only negative-cache when both endpoints succeeded - Clear negative-cached titles on user-initiated refresh so posts published after first sight can resolve - Cap unattended load-more auto-advance at 3 pages and let it handle empty pages, not just fully-deduplicated ones - Don't cancel/re-fire an identical in-flight title-resolve batch - Add CommentsRsDataSourceTest covering the cache, fallback, and chunking behavior, plus ViewModel tests for the new paging rules Co-Authored-By: Claude Opus 4.8 (1M context) --- .../comments/unified/CommentsRsDataSource.kt | 80 +++++---- .../ui/commentsrs/CommentsRsListViewModel.kt | 44 +++-- .../unified/CommentsRsDataSourceTest.kt | 157 ++++++++++++++++++ .../commentsrs/CommentsRsListViewModelTest.kt | 57 +++++++ 4 files changed, 296 insertions(+), 42 deletions(-) create mode 100644 WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt diff --git a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt index 44bebc60297c..c3a1df5ff359 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt @@ -137,54 +137,69 @@ class CommentsRsDataSource @Inject constructor( if (uncached.isEmpty()) return result safe(errorValue = Unit) { - val unresolved = fetchTitlesFromEndpoint(site, PostEndpointType.Posts, uncached, result) - ?: return@safe - val stillUnresolved = if (unresolved.isEmpty()) { - unresolved + val postsSucceeded = fetchTitlesFromEndpoint(site, PostEndpointType.Posts, uncached, result) + val remaining = uncached.filter { it !in result } + val pagesSucceeded = if (remaining.isEmpty()) { + true } else { - fetchTitlesFromEndpoint(site, PostEndpointType.Pages, unresolved, result) ?: return@safe + fetchTitlesFromEndpoint(site, PostEndpointType.Pages, remaining, result) } - // Only reached when both endpoints succeeded, so these ids genuinely have no - // resolvable title (as opposed to a transient request failure). - for (id in stillUnresolved) { - postTitleCache[site.id to id] = "" - result[id] = "" + // Only when both endpoints succeeded are the leftover ids genuinely unresolvable + // (custom post types, non-published posts) rather than a transient failure. + if (postsSucceeded && pagesSucceeded) { + for (id in uncached.filter { it !in result }) { + postTitleCache[site.id to id] = "" + result[id] = "" + } } } return result } /** - * Fetches titles for [ids] from [endpointType] into [into] (and the cache), returning the ids - * the endpoint didn't include, or null when the request failed. [PostListParams.perPage] is - * set explicitly: the server default of 10 would silently truncate larger batches. + * Forgets negative-cached (unresolvable) titles for [site] so they're retried, e.g. on a + * user-initiated refresh — a post that wasn't published when first seen may be by now. + */ + fun clearUnresolvedPostTitles(site: SiteModel) { + postTitleCache.entries.removeIf { it.key.first == site.id && it.value.isEmpty() } + } + + /** + * Fetches titles for [ids] from [endpointType] into [into] (and the cache), returning whether + * every request succeeded. [PostListParams.perPage] is set explicitly (the server default of + * 10 would silently truncate larger batches) and ids are chunked to the server's per_page + * maximum of 100 (a larger value is rejected outright with `rest_invalid_param`). */ private suspend fun fetchTitlesFromEndpoint( site: SiteModel, endpointType: PostEndpointType, ids: List, into: MutableMap - ): List? { + ): Boolean { val client = wpApiClientProvider.getWpApiClient(site) - val response = client.request { - it.posts().filterListWithViewContext( - endpointType, - PostListParams(include = ids, perPage = ids.size.toUInt()), - listOf(SparseAnyPostFieldWithViewContext.ID, SparseAnyPostFieldWithViewContext.TITLE) - ) - } - if (response !is WpRequestResult.Success) { - val message = (response as? WpRequestResult.WpError<*>)?.errorMessage - AppLog.w(AppLog.T.COMMENTS, "fetchPostTitles ($endpointType) failed: $message") - return null - } - for (post in response.response.data) { - val id = post.id ?: continue - val title = post.title?.rendered.orEmpty() - postTitleCache[site.id to id] = title - into[id] = title + var allSucceeded = true + for (chunk in ids.chunked(MAX_TITLES_PER_REQUEST)) { + val response = client.request { + it.posts().filterListWithViewContext( + endpointType, + PostListParams(include = chunk, perPage = chunk.size.toUInt()), + listOf(SparseAnyPostFieldWithViewContext.ID, SparseAnyPostFieldWithViewContext.TITLE) + ) + } + if (response is WpRequestResult.Success) { + for (post in response.response.data) { + val id = post.id ?: continue + val title = post.title?.rendered.orEmpty() + postTitleCache[site.id to id] = title + into[id] = title + } + } else { + val message = (response as? WpRequestResult.WpError<*>)?.errorMessage + AppLog.w(AppLog.T.COMMENTS, "fetchPostTitles ($endpointType) failed: $message") + allSucceeded = false + } } - return ids.filter { it !in into } + return allSucceeded } suspend fun updateStatus(site: SiteModel, commentId: Long, status: CommentStatus): RsResult = @@ -236,6 +251,7 @@ class CommentsRsDataSource @Inject constructor( companion object { internal const val COMMENTS_PAGE_SIZE = 30u + private const val MAX_TITLES_PER_REQUEST = 100 } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt index 3f9711f2b716..7a4bfd7912d2 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt @@ -62,7 +62,10 @@ class CommentsRsListViewModel @Inject constructor( // bump paged from a cursor that predates the new page 1, so it's stale and gets discarded. private val pageGenerations = mutableMapOf() private val initializingTabs = mutableSetOf() - private val resolveTitleJobs = mutableMapOf() + + // The in-flight title-resolve job per tab, with the ids it's resolving so an identical + // request isn't cancelled and re-fired when an applied page adds nothing new. + private val resolveTitleJobs = mutableMapOf>>() private val _site: SiteModel? = selectedSiteRepository.getSelectedSite() private val site: SiteModel @@ -92,6 +95,9 @@ class CommentsRsListViewModel @Inject constructor( @MainThread fun refreshTab(tab: CommentsRsListTab, isUserRefresh: Boolean = false) { if (!_tabStates.value.containsKey(tab)) return + // An explicit refresh also retries post titles that previously resolved as + // unresolvable — e.g. a post that has been published since it was first seen. + if (isUserRefresh) commentsRsDataSource.clearUnresolvedPostTitles(site) updateTabUiState(tab) { copy(isRefreshing = isUserRefresh, error = null) } fetchFirstPage(tab) } @@ -105,6 +111,10 @@ class CommentsRsListViewModel @Inject constructor( /** Loads the next page for [tab] if there is one and nothing else is in flight. */ @MainThread fun loadMore(tab: CommentsRsListTab) { + loadMoreInternal(tab, autoAdvanceDepth = 0) + } + + private fun loadMoreInternal(tab: CommentsRsListTab, autoAdvanceDepth: Int) { val current = getTabUiState(tab) val params = nextPageParams[tab] val isBusy = current.isLoading || current.isRefreshing || current.isLoadingMore @@ -124,14 +134,16 @@ class CommentsRsListViewModel @Inject constructor( is RsCommentsPageResult.Success -> { val sizeBefore = getTabUiState(tab).comments.size applyPage(tab, result, append = true) - // When the whole page deduped away (the list shifted server-side), the - // scroll position hasn't changed so the load-more trigger won't re-fire; - // advance to the next page directly. - if (result.comments.isNotEmpty() && - getTabUiState(tab).comments.size == sizeBefore && - nextPageParams[tab] != null + // When a page adds no rows (it deduped away after a server-side shift, or + // came back empty mid-shrink), the scroll position hasn't changed so the + // load-more trigger won't re-fire; advance to the next page directly. The + // depth cap keeps a pathological cursor chain (e.g. a proxy ignoring the + // page param) from firing unbounded unattended requests. + if (getTabUiState(tab).comments.size == sizeBefore && + nextPageParams[tab] != null && + autoAdvanceDepth < MAX_AUTO_ADVANCE_PAGES ) { - loadMore(tab) + loadMoreInternal(tab, autoAdvanceDepth + 1) } } is RsCommentsPageResult.Error -> { @@ -230,14 +242,20 @@ class CommentsRsListViewModel @Inject constructor( .distinct() if (unresolvedIds.isEmpty()) return - resolveTitleJobs[tab]?.cancel() - resolveTitleJobs[tab] = viewModelScope.launch { + // An applied page that added no rows (deduped away) leaves the same ids unresolved; + // let the in-flight request finish rather than cancelling and re-firing it. + val inFlight = resolveTitleJobs[tab] + if (inFlight != null && inFlight.first.isActive && inFlight.second == unresolvedIds) return + + inFlight?.first?.cancel() + val job = viewModelScope.launch { val titles = withContext(bgDispatcher) { commentsRsDataSource.fetchPostTitles(site, unresolvedIds) } if (titles.isEmpty()) return@launch updateTabUiState(tab) { copy(comments = comments.map { it.copy(postTitle = titles[it.postId] ?: it.postTitle) }) } } + resolveTitleJobs[tab] = job to unresolvedIds } /** The server message when it sent one, otherwise a friendly offline/generic message. */ @@ -261,4 +279,10 @@ class CommentsRsListViewModel @Inject constructor( private fun updateTabUiState(tab: CommentsRsListTab, update: CommentsTabUiState.() -> CommentsTabUiState) { _tabStates.value += (tab to getTabUiState(tab).update()) } + + companion object { + // How many extra pages a single load-more may fetch unattended when applied pages + // add no rows, before waiting for the next user scroll. + private const val MAX_AUTO_ADVANCE_PAGES = 3 + } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt new file mode 100644 index 000000000000..475eddd0fd38 --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt @@ -0,0 +1,157 @@ +package org.wordpress.android.ui.comments.unified + +import kotlinx.coroutines.test.runTest +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Test +import org.mockito.kotlin.any +import org.mockito.kotlin.anyOrNull +import org.mockito.kotlin.mock +import org.mockito.kotlin.times +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider +import rs.wordpress.api.kotlin.WpApiClient +import rs.wordpress.api.kotlin.WpRequestResult +import uniffi.wp_api.PostsRequestFilterListWithViewContextResponse +import uniffi.wp_api.RequestMethod +import uniffi.wp_api.SparseAnyPostWithViewContext +import uniffi.wp_api.SparsePostTitleWithViewContext +import uniffi.wp_api.WpErrorCode + +/** + * Tests for [CommentsRsDataSource.fetchPostTitles]: the per-site title cache, the posts→pages + * endpoint fallback, negative caching of unresolvable ids, and request chunking. + */ +class CommentsRsDataSourceTest { + private val wpApiClient: WpApiClient = mock() + private val wpApiClientProvider: WpApiClientProvider = mock() + private lateinit var dataSource: CommentsRsDataSource + + private val siteA = SiteModel().apply { id = 1 } + private val siteB = SiteModel().apply { id = 2 } + + @Before + fun setUp() { + dataSource = CommentsRsDataSource(wpApiClientProvider) + whenever(wpApiClientProvider.getWpApiClient(any(), anyOrNull())).thenReturn(wpApiClient) + } + + @Test + fun `resolved titles are cached and skip the network`() = runTest { + stubRequests(successResponse(sparsePost(5, "Hello"))) + + assertThat(dataSource.fetchPostTitles(siteA, listOf(5))).isEqualTo(mapOf(5L to "Hello")) + assertThat(dataSource.fetchPostTitles(siteA, listOf(5))).isEqualTo(mapOf(5L to "Hello")) + + verify(wpApiClient, times(1)).request(any()) + } + + @Test + fun `titles are cached per site, not just per post id`() = runTest { + stubRequests( + successResponse(sparsePost(5, "Site A post")), + successResponse(sparsePost(5, "Site B post")) + ) + + assertThat(dataSource.fetchPostTitles(siteA, listOf(5))).isEqualTo(mapOf(5L to "Site A post")) + assertThat(dataSource.fetchPostTitles(siteB, listOf(5))).isEqualTo(mapOf(5L to "Site B post")) + + verify(wpApiClient, times(2)).request(any()) + } + + @Test + fun `a posts endpoint failure still tries the pages endpoint`() = runTest { + stubRequests(wpError(), successResponse(sparsePost(7, "About"))) + + assertThat(dataSource.fetchPostTitles(siteA, listOf(7))).isEqualTo(mapOf(7L to "About")) + + verify(wpApiClient, times(2)).request(any()) + } + + @Test + fun `ids neither endpoint returns are negative-cached once both succeed`() = runTest { + stubRequests(successResponse(), successResponse()) + + assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "")) + assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "")) + + // Two requests (posts + pages) for the first call, none for the second. + verify(wpApiClient, times(2)).request(any()) + } + + @Test + fun `a transient failure is not negative-cached`() = runTest { + stubRequests( + successResponse(), // posts: id not found + wpError(), // pages: transient failure — must NOT negative-cache + successResponse(sparsePost(9, "Resolved later")) + ) + + assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEmpty() + assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "Resolved later")) + + verify(wpApiClient, times(3)).request(any()) + } + + @Test + fun `clearUnresolvedPostTitles retries negative-cached ids`() = runTest { + stubRequests( + successResponse(), // posts: not found + successResponse(), // pages: not found → negative-cached + successResponse(sparsePost(9, "Now published")) + ) + assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "")) + + dataSource.clearUnresolvedPostTitles(siteA) + + assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "Now published")) + verify(wpApiClient, times(3)).request(any()) + } + + @Test + fun `batches over 100 ids are chunked to the per_page maximum`() = runTest { + val ids = (1L..101L).toList() + stubRequests( + successResponse(*ids.take(100).map { sparsePost(it, "Post $it") }.toTypedArray()), + successResponse(sparsePost(101, "Post 101")) + ) + + val titles = dataSource.fetchPostTitles(siteA, ids) + + assertThat(titles).hasSize(101) + assertThat(titles[101L]).isEqualTo("Post 101") + verify(wpApiClient, times(2)).request(any()) + } + + private fun sparsePost(id: Long, title: String): SparseAnyPostWithViewContext { + val sparseTitle = mock() + whenever(sparseTitle.rendered).thenReturn(title) + val post = mock() + whenever(post.id).thenReturn(id) + whenever(post.title).thenReturn(sparseTitle) + return post + } + + private fun successResponse(vararg posts: SparseAnyPostWithViewContext) = WpRequestResult.Success( + response = PostsRequestFilterListWithViewContextResponse(posts.toList(), mock(), null, null) + ) + + private fun wpError() = WpRequestResult.WpError( + errorCode = WpErrorCode.Forbidden(), + errorMessage = "server said no", + statusCode = 403u, + response = "", + requestUrl = "https://example.com", + requestMethod = RequestMethod.GET + ) + + @Suppress("UNCHECKED_CAST") + private suspend fun stubRequests(vararg responses: WpRequestResult<*>) { + var stubbing = whenever(wpApiClient.request(any())) + for (response in responses) { + stubbing = stubbing.thenReturn(response as WpRequestResult) + } + } +} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt index 825934f3ac64..e448ad830fd4 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt @@ -13,6 +13,7 @@ import org.mockito.Mock import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.eq +import org.mockito.kotlin.never import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever @@ -232,6 +233,62 @@ class CommentsRsListViewModelTest : BaseUnitTest(StandardTestDispatcher()) { assertThat(state.canLoadMore).isFalse() } + @Test + fun `an empty page with a remaining cursor auto-advances to the next page`() = test { + givenPage(listOf(rsItem(id = 1), rsItem(id = 2)), nextPageParams = NEXT_PAGE) + val viewModel = createViewModel() + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + // A middle page can come back empty (comments deleted server-side) while the + // headers still advertise more pages. + whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())).thenReturn( + RsCommentsPageResult.Success(emptyList(), THIRD_PAGE), + RsCommentsPageResult.Success(listOf(rsItem(id = 3)), null) + ) + + viewModel.loadMore(CommentsRsListTab.ALL) + advanceUntilIdle() + + val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) + assertThat(state.comments.map { it.remoteCommentId }).containsExactly(1L, 2L, 3L) + assertThat(state.canLoadMore).isFalse() + } + + @Test + fun `auto-advance is capped when pages keep adding nothing`() = test { + givenPage(listOf(rsItem(id = 1)), nextPageParams = NEXT_PAGE) + val viewModel = createViewModel() + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + // Pathological cursor chain: every page dedupes away with a cursor still present. + whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())) + .thenReturn(RsCommentsPageResult.Success(listOf(rsItem(id = 1)), NEXT_PAGE)) + + viewModel.loadMore(CommentsRsListTab.ALL) + advanceUntilIdle() + + // 1 init + 1 user loadMore + at most 3 auto-advances, then the chain stops. + verify(commentsRsDataSource, times(5)).fetchCommentsPage(eq(site), any()) + val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) + assertThat(state.isLoadingMore).isFalse() + } + + @Test + fun `a user refresh retries unresolvable post titles, a silent refresh does not`() = test { + givenPage(listOf(rsItem(id = 1))) + val viewModel = createViewModel() + viewModel.initTab(CommentsRsListTab.ALL) + advanceUntilIdle() + + viewModel.refreshTab(CommentsRsListTab.ALL) // silent + advanceUntilIdle() + verify(commentsRsDataSource, never()).clearUnresolvedPostTitles(any()) + + viewModel.refreshTab(CommentsRsListTab.ALL, isUserRefresh = true) + advanceUntilIdle() + verify(commentsRsDataSource).clearUnresolvedPostTitles(site) + } + @Test fun `loadMore is a no-op when there is no next page`() = test { givenPage(listOf(rsItem(id = 1)), nextPageParams = null) From ca271c875262da5fc33375720160a5535a8f3ee6 Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Thu, 2 Jul 2026 16:04:41 -0400 Subject: [PATCH 07/20] Close title-resolve race windows and strengthen data source tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Cancel a tab's in-flight title-resolve job on user refresh so a job holding pre-clear 'not found' results can't re-apply them via the identical-ids guard - Generation-stamp the negative cache so a fetch already in flight when the user cleared can't re-poison entries with pre-clear results - Make the data source tests execute each request's builder lambda against a mocked uniffi client, asserting the endpoint, include count, and perPage actually sent — the chunking test now fails if chunking is reverted, and the posts-before-pages order is asserted, not assumed Co-Authored-By: Claude Opus 4.8 (1M context) --- .../comments/unified/CommentsRsDataSource.kt | 12 +- .../ui/commentsrs/CommentsRsListViewModel.kt | 12 +- .../unified/CommentsRsDataSourceTest.kt | 107 +++++++++++++++--- .../commentsrs/CommentsRsListViewModelTest.kt | 25 ++++ 4 files changed, 135 insertions(+), 21 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt index c3a1df5ff359..631f935c5f9a 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSource.kt @@ -42,6 +42,11 @@ class CommentsRsDataSource @Inject constructor( // whose title can't be resolved (custom post types, non-published posts). private val postTitleCache = ConcurrentHashMap, String>() + // Bumped by [clearUnresolvedPostTitles]: a fetch already in flight when the user cleared + // would otherwise re-poison the cache with its pre-clear "not found" results. + @Volatile + private var unresolvedCacheGeneration = 0 + data class RsComment( val authorName: String, val authorAvatarUrl: String, @@ -137,6 +142,7 @@ class CommentsRsDataSource @Inject constructor( if (uncached.isEmpty()) return result safe(errorValue = Unit) { + val generation = unresolvedCacheGeneration val postsSucceeded = fetchTitlesFromEndpoint(site, PostEndpointType.Posts, uncached, result) val remaining = uncached.filter { it !in result } val pagesSucceeded = if (remaining.isEmpty()) { @@ -145,8 +151,9 @@ class CommentsRsDataSource @Inject constructor( fetchTitlesFromEndpoint(site, PostEndpointType.Pages, remaining, result) } // Only when both endpoints succeeded are the leftover ids genuinely unresolvable - // (custom post types, non-published posts) rather than a transient failure. - if (postsSucceeded && pagesSucceeded) { + // (custom post types, non-published posts) rather than a transient failure — and + // only when no clear happened mid-flight, so pre-clear results can't re-poison it. + if (postsSucceeded && pagesSucceeded && generation == unresolvedCacheGeneration) { for (id in uncached.filter { it !in result }) { postTitleCache[site.id to id] = "" result[id] = "" @@ -161,6 +168,7 @@ class CommentsRsDataSource @Inject constructor( * user-initiated refresh — a post that wasn't published when first seen may be by now. */ fun clearUnresolvedPostTitles(site: SiteModel) { + unresolvedCacheGeneration++ postTitleCache.entries.removeIf { it.key.first == site.id && it.value.isEmpty() } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt index 7a4bfd7912d2..8b436a71238d 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt @@ -95,9 +95,15 @@ class CommentsRsListViewModel @Inject constructor( @MainThread fun refreshTab(tab: CommentsRsListTab, isUserRefresh: Boolean = false) { if (!_tabStates.value.containsKey(tab)) return - // An explicit refresh also retries post titles that previously resolved as - // unresolvable — e.g. a post that has been published since it was first seen. - if (isUserRefresh) commentsRsDataSource.clearUnresolvedPostTitles(site) + if (isUserRefresh) { + // An explicit refresh also retries post titles that previously resolved as + // unresolvable — e.g. a post that has been published since it was first seen. + commentsRsDataSource.clearUnresolvedPostTitles(site) + // A resolve job that started before the clear may apply pre-clear "not found" + // titles; cancel it so the post-refresh pass fetches fresh results instead of + // deferring to it via the identical-ids guard. + resolveTitleJobs[tab]?.first?.cancel() + } updateTabUiState(tab) { copy(isRefreshing = isUserRefresh, error = null) } fetchFirstPage(tab) } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt index 475eddd0fd38..e704d2361259 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt @@ -6,29 +6,51 @@ import org.junit.Before import org.junit.Test import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull +import org.mockito.kotlin.doSuspendableAnswer import org.mockito.kotlin.mock -import org.mockito.kotlin.times -import org.mockito.kotlin.verify +import org.mockito.kotlin.stub import org.mockito.kotlin.whenever import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider import rs.wordpress.api.kotlin.WpApiClient import rs.wordpress.api.kotlin.WpRequestResult +import uniffi.wp_api.PostEndpointType +import uniffi.wp_api.PostListParams +import uniffi.wp_api.PostsRequestExecutor import uniffi.wp_api.PostsRequestFilterListWithViewContextResponse import uniffi.wp_api.RequestMethod import uniffi.wp_api.SparseAnyPostWithViewContext import uniffi.wp_api.SparsePostTitleWithViewContext +import uniffi.wp_api.UniffiWpApiClient import uniffi.wp_api.WpErrorCode /** * Tests for [CommentsRsDataSource.fetchPostTitles]: the per-site title cache, the posts→pages * endpoint fallback, negative caching of unresolvable ids, and request chunking. + * + * The [wpApiClient] stub executes each request's builder lambda against a mocked + * [UniffiWpApiClient], recording the endpoint and paging params actually sent — so the tests + * pin the requests made, not just how many there were. */ class CommentsRsDataSourceTest { - private val wpApiClient: WpApiClient = mock() + private data class RecordedRequest( + val endpointType: PostEndpointType, + val includeCount: Int, + val perPage: UInt? + ) + private val wpApiClientProvider: WpApiClientProvider = mock() + private val wpApiClient: WpApiClient = mock() + private val uniffiClient: UniffiWpApiClient = mock() + private val postsExecutor: PostsRequestExecutor = mock() private lateinit var dataSource: CommentsRsDataSource + private val recordedRequests = mutableListOf() + private val cannedResults = ArrayDeque>() + + /** Invoked after each request completes; lets a test simulate concurrent work mid-fetch. */ + private var afterRequest: (() -> Unit)? = null + private val siteA = SiteModel().apply { id = 1 } private val siteB = SiteModel().apply { id = 2 } @@ -36,6 +58,29 @@ class CommentsRsDataSourceTest { fun setUp() { dataSource = CommentsRsDataSource(wpApiClientProvider) whenever(wpApiClientProvider.getWpApiClient(any(), anyOrNull())).thenReturn(wpApiClient) + whenever(uniffiClient.posts()).thenReturn(postsExecutor) + postsExecutor.stub { + on { filterListWithViewContext(any(), any(), any()) } doSuspendableAnswer { invocation -> + val params = invocation.getArgument(1) + recordedRequests += RecordedRequest( + endpointType = invocation.getArgument(0), + includeCount = params.include.size, + perPage = params.perPage + ) + // The payload is decided by the request-level stub below; this value is unused. + mock() + } + } + wpApiClient.stub { + on { request(any()) } doSuspendableAnswer { invocation -> + // Run the builder lambda so the executor stub above records what was requested. + val executor = invocation.getArgument Any>(0) + executor(uniffiClient) + val result = cannedResults.removeFirst() + afterRequest?.invoke() + result + } + } } @Test @@ -45,7 +90,7 @@ class CommentsRsDataSourceTest { assertThat(dataSource.fetchPostTitles(siteA, listOf(5))).isEqualTo(mapOf(5L to "Hello")) assertThat(dataSource.fetchPostTitles(siteA, listOf(5))).isEqualTo(mapOf(5L to "Hello")) - verify(wpApiClient, times(1)).request(any()) + assertThat(recordedRequests).hasSize(1) } @Test @@ -58,7 +103,7 @@ class CommentsRsDataSourceTest { assertThat(dataSource.fetchPostTitles(siteA, listOf(5))).isEqualTo(mapOf(5L to "Site A post")) assertThat(dataSource.fetchPostTitles(siteB, listOf(5))).isEqualTo(mapOf(5L to "Site B post")) - verify(wpApiClient, times(2)).request(any()) + assertThat(recordedRequests).hasSize(2) } @Test @@ -67,7 +112,8 @@ class CommentsRsDataSourceTest { assertThat(dataSource.fetchPostTitles(siteA, listOf(7))).isEqualTo(mapOf(7L to "About")) - verify(wpApiClient, times(2)).request(any()) + assertThat(recordedRequests.map { it.endpointType }) + .containsExactly(PostEndpointType.Posts, PostEndpointType.Pages) } @Test @@ -77,8 +123,9 @@ class CommentsRsDataSourceTest { assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "")) assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "")) - // Two requests (posts + pages) for the first call, none for the second. - verify(wpApiClient, times(2)).request(any()) + // Posts + pages for the first call, none for the second. + assertThat(recordedRequests.map { it.endpointType }) + .containsExactly(PostEndpointType.Posts, PostEndpointType.Pages) } @Test @@ -92,7 +139,8 @@ class CommentsRsDataSourceTest { assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEmpty() assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "Resolved later")) - verify(wpApiClient, times(3)).request(any()) + assertThat(recordedRequests.map { it.endpointType }) + .containsExactly(PostEndpointType.Posts, PostEndpointType.Pages, PostEndpointType.Posts) } @Test @@ -107,7 +155,29 @@ class CommentsRsDataSourceTest { dataSource.clearUnresolvedPostTitles(siteA) assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "Now published")) - verify(wpApiClient, times(3)).request(any()) + assertThat(recordedRequests).hasSize(3) + } + + @Test + fun `a clear during an in-flight fetch prevents negative caching`() = runTest { + stubRequests( + successResponse(), // posts: not found + successResponse(), // pages: not found — but a clear lands before the cache write + successResponse(), // retry: posts + successResponse() // retry: pages + ) + afterRequest = { + // Simulates another surface clearing while this fetch is between its network + // responses and its negative-cache write. + if (recordedRequests.size == 2) dataSource.clearUnresolvedPostTitles(siteA) + } + + assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEmpty() + + // Not negative-cached, so a later fetch goes back to the network. + afterRequest = null + assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "")) + assertThat(recordedRequests).hasSize(4) } @Test @@ -122,7 +192,13 @@ class CommentsRsDataSourceTest { assertThat(titles).hasSize(101) assertThat(titles[101L]).isEqualTo("Post 101") - verify(wpApiClient, times(2)).request(any()) + // Two posts-endpoint requests with the batch split at 100, each with a matching + // explicit perPage — NOT one oversized request plus a pages fallback. + assertThat(recordedRequests.map { Triple(it.endpointType, it.includeCount, it.perPage) }) + .containsExactly( + Triple(PostEndpointType.Posts, 100, 100u), + Triple(PostEndpointType.Posts, 1, 1u) + ) } private fun sparsePost(id: Long, title: String): SparseAnyPostWithViewContext { @@ -148,10 +224,9 @@ class CommentsRsDataSourceTest { ) @Suppress("UNCHECKED_CAST") - private suspend fun stubRequests(vararg responses: WpRequestResult<*>) { - var stubbing = whenever(wpApiClient.request(any())) - for (response in responses) { - stubbing = stubbing.thenReturn(response as WpRequestResult) - } + private fun stubRequests(vararg responses: WpRequestResult<*>) { + recordedRequests.clear() + cannedResults.clear() + responses.forEach { cannedResults.add(it as WpRequestResult) } } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt index e448ad830fd4..dbc2d9fe4fa2 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt @@ -4,7 +4,9 @@ import androidx.lifecycle.viewModelScope import app.cash.turbine.test import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.cancel +import kotlinx.coroutines.delay import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.runCurrent import org.assertj.core.api.Assertions.assertThat import org.junit.After import org.junit.Before @@ -12,6 +14,7 @@ import org.junit.Test import org.mockito.Mock import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull +import org.mockito.kotlin.doSuspendableAnswer import org.mockito.kotlin.eq import org.mockito.kotlin.never import org.mockito.kotlin.times @@ -289,6 +292,28 @@ class CommentsRsListViewModelTest : BaseUnitTest(StandardTestDispatcher()) { verify(commentsRsDataSource).clearUnresolvedPostTitles(site) } + @Test + fun `a user refresh cancels an in-flight title resolve and fetches fresh titles`() = test { + givenPage(listOf(rsItem(id = 1, postId = 10))) + // The first resolve hangs mid-flight (holding pre-refresh results); the retry after + // the user refresh returns the fresh title. + whenever(commentsRsDataSource.fetchPostTitles(eq(site), any())) + .doSuspendableAnswer { delay(60_000); emptyMap() } + .thenReturn(mapOf(10L to "Fresh title")) + val viewModel = createViewModel() + viewModel.initTab(CommentsRsListTab.ALL) + runCurrent() // page applied; resolve job now suspended mid-flight + + viewModel.refreshTab(CommentsRsListTab.ALL, isUserRefresh = true) + advanceUntilIdle() + + // Without the cancel, the identical-ids guard defers to the hung pre-refresh job and + // the fresh title is never fetched. + val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) + assertThat(state.comments.first().postTitle).isEqualTo("Fresh title") + verify(commentsRsDataSource, times(2)).fetchPostTitles(eq(site), any()) + } + @Test fun `loadMore is a no-op when there is no next page`() = test { givenPage(listOf(rsItem(id = 1)), nextPageParams = null) From e666ee450945e52af068ecd08dc1abf274e43453 Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Thu, 2 Jul 2026 16:29:33 -0400 Subject: [PATCH 08/20] Construct uniffi data classes in tests instead of mocking them Resolves the Android Lint code-scanning alerts on CommentsRsDataSourceTest: SparseAnyPostWithViewContext and PostsRequestFilterListWithViewContextResponse are pure data classes, so the tests now build real instances. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../unified/CommentsRsDataSourceTest.kt | 39 ++++++++++++++----- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt index e704d2361259..a313fa3bbe52 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt @@ -68,7 +68,7 @@ class CommentsRsDataSourceTest { perPage = params.perPage ) // The payload is decided by the request-level stub below; this value is unused. - mock() + PostsRequestFilterListWithViewContextResponse(emptyList(), mock(), null, null) } } wpApiClient.stub { @@ -201,14 +201,35 @@ class CommentsRsDataSourceTest { ) } - private fun sparsePost(id: Long, title: String): SparseAnyPostWithViewContext { - val sparseTitle = mock() - whenever(sparseTitle.rendered).thenReturn(title) - val post = mock() - whenever(post.id).thenReturn(id) - whenever(post.title).thenReturn(sparseTitle) - return post - } + // A sparse post as returned by the id+title sparse-field request: everything else null. + private fun sparsePost(id: Long, title: String) = SparseAnyPostWithViewContext( + id = id, + date = null, + dateGmt = null, + guid = null, + link = null, + modified = null, + modifiedGmt = null, + slug = null, + status = null, + postType = null, + title = SparsePostTitleWithViewContext(rendered = title), + content = null, + author = null, + excerpt = null, + featuredMedia = null, + commentStatus = null, + pingStatus = null, + format = null, + meta = null, + sticky = null, + template = null, + categories = null, + tags = null, + parent = null, + menuOrder = null, + additionalFields = null + ) private fun successResponse(vararg posts: SparseAnyPostWithViewContext) = WpRequestResult.Success( response = PostsRequestFilterListWithViewContextResponse(posts.toList(), mock(), null, null) From 89e2b61299baee0e02f0e463bef67713d235e00c Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Fri, 3 Jul 2026 08:13:25 -0400 Subject: [PATCH 09/20] Fix review findings and slim the list PR - Detail opened from the rs list backfills the FluxC cache (WP.com) and resolves the post title via rs, so title and like state are correct - Detail reports RESULT_OK only when the comment changed; the list only refreshes then, keeping paged lists and scroll position intact - Guard against duplicate first-page fetches; skip the retry snackbar when a silent refresh fails on a tab that already shows comments - Clear isLoadingMore when a refresh replaces the list so paging can't stall behind the busy guard - Stop the load-more trigger firing on the empty pre-layout list and re-arm it when the list size changes - Evict all cached post titles for the site on user refresh so renamed posts pick up their new title - Track COMMENT_FILTER_CHANGED on tab changes like the legacy list - Restore the singleTop launch mode the legacy comments activity has - Drop the dead initializingTabs bookkeeping - Merge RsCommentListItem into RsComment (one shared type and mapper) - Move the new unit tests to a follow-up PR to reduce this PR's size Co-Authored-By: Claude Fable 5 --- WordPress/src/main/AndroidManifest.xml | 1 + .../comments/unified/CommentsRsDataSource.kt | 48 +- .../unified/UnifiedCommentDetailsFragment.kt | 8 +- .../unified/UnifiedCommentDetailsViewModel.kt | 50 ++- .../ui/commentsrs/CommentsRsListActivity.kt | 12 +- .../ui/commentsrs/CommentsRsListTab.kt | 7 + .../ui/commentsrs/CommentsRsListViewModel.kt | 114 +++-- .../screens/CommentsRsListScreen.kt | 2 + .../screens/CommentsRsTabListScreen.kt | 9 +- .../unified/CommentsRsDataSourceTest.kt | 253 ----------- .../unified/CommentsRsListMappingTest.kt | 110 ----- .../UnifiedCommentDetailsViewModelTest.kt | 5 +- .../ui/commentsrs/CommentsRsListTabTest.kt | 26 -- .../commentsrs/CommentsRsListViewModelTest.kt | 414 ------------------ 14 files changed, 162 insertions(+), 897 deletions(-) delete mode 100644 WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt delete mode 100644 WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsListMappingTest.kt delete mode 100644 WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListTabTest.kt delete mode 100644 WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt diff --git a/WordPress/src/main/AndroidManifest.xml b/WordPress/src/main/AndroidManifest.xml index 712f239dc07c..6131afb24c14 100644 --- a/WordPress/src/main/AndroidManifest.xml +++ b/WordPress/src/main/AndroidManifest.xml @@ -316,6 +316,7 @@ , String>() - // Bumped by [clearUnresolvedPostTitles]: a fetch already in flight when the user cleared - // would otherwise re-poison the cache with its pre-clear "not found" results. + // Bumped by [clearPostTitles]: a fetch already in flight when the user cleared would + // otherwise re-poison the cache with its pre-clear "not found" results. @Volatile private var unresolvedCacheGeneration = 0 + /** A comment mapped from [CommentWithViewContext], shared by the detail and list screens. */ data class RsComment( - val authorName: String, - val authorAvatarUrl: String, - val dateGmt: Date, - val contentHtml: String, - val url: String, - val postId: Long, - val status: CommentStatus - ) - - /** One comment list row, mapped from [CommentWithViewContext]. */ - data class RsCommentListItem( val remoteCommentId: Long, val authorName: String, val authorAvatarUrl: String, val dateGmt: Date, val contentHtml: String, + val url: String, val postId: Long, val status: CommentStatus ) @@ -77,7 +68,7 @@ class CommentsRsDataSource @Inject constructor( /** Result of fetching one page of the comment list. */ sealed interface RsCommentsPageResult { data class Success( - val comments: List, + val comments: List, /** Ready-made params for the next page; null when this was the last page. */ val nextPageParams: CommentListParams? ) : RsCommentsPageResult @@ -106,7 +97,7 @@ class CommentsRsDataSource @Inject constructor( val client = wpApiClientProvider.getWpApiClient(site) when (val result = client.request { it.comments().listWithViewContext(params) }) { is WpRequestResult.Success -> RsCommentsPageResult.Success( - comments = result.response.data.map { it.toRsCommentListItem() }, + comments = result.response.data.map { it.toRsComment() }, nextPageParams = result.response.nextPageParams ) is WpRequestResult.WpError -> RsCommentsPageResult.Error(result.errorMessage) @@ -164,12 +155,14 @@ class CommentsRsDataSource @Inject constructor( } /** - * Forgets negative-cached (unresolvable) titles for [site] so they're retried, e.g. on a - * user-initiated refresh — a post that wasn't published when first seen may be by now. + * Forgets all cached titles for [site] so they're re-fetched, e.g. on a user-initiated + * refresh — a post that wasn't published when first seen may be by now, and a resolved + * title may have been renamed since it was cached. Fetches are batched, so re-resolving a + * page of titles costs a single request. */ - fun clearUnresolvedPostTitles(site: SiteModel) { + fun clearPostTitles(site: SiteModel) { unresolvedCacheGeneration++ - postTitleCache.entries.removeIf { it.key.first == site.id && it.value.isEmpty() } + postTitleCache.entries.removeIf { it.key.first == site.id } } /** @@ -245,30 +238,21 @@ class CommentsRsDataSource @Inject constructor( errorValue } - private fun CommentWithViewContext.toRsComment() = RsComment( - authorName = authorName, - authorAvatarUrl = pickAvatarUrl(), - // dateGmt is a UTC java.util.Date (an absolute instant), so relative-time formatting is - // correct regardless of the site's timezone — unlike the offset-less local `date` field. - dateGmt = dateGmt, - contentHtml = content.rendered, - url = link, - postId = post, - status = status.toAppCommentStatus() - ) - companion object { internal const val COMMENTS_PAGE_SIZE = 30u private const val MAX_TITLES_PER_REQUEST = 100 } } -internal fun CommentWithViewContext.toRsCommentListItem() = CommentsRsDataSource.RsCommentListItem( +internal fun CommentWithViewContext.toRsComment() = CommentsRsDataSource.RsComment( remoteCommentId = id, authorName = authorName, authorAvatarUrl = pickAvatarUrl(), + // dateGmt is a UTC java.util.Date (an absolute instant), so relative-time formatting is + // correct regardless of the site's timezone — unlike the offset-less local `date` field. dateGmt = dateGmt, contentHtml = content.rendered, + url = link, postId = post, status = status.toAppCommentStatus() ) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsFragment.kt index d5870313a1ab..15e62225be12 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsFragment.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsFragment.kt @@ -102,7 +102,7 @@ class UnifiedCommentDetailsFragment : private val editCommentLauncher: ActivityResultLauncher = registerForActivityResult(StartActivityForResult()) { result -> if (result.resultCode == AppCompatActivity.RESULT_OK) { - viewModel.refreshComment() + viewModel.onCommentEdited() } } @@ -234,6 +234,12 @@ class UnifiedCommentDetailsFragment : } viewModel.onSnackbarMessage.observeEvent(viewLifecycleOwner) { showSnackbar(it) } + + // Report the change to the launching screen (the rs comments list only refreshes when + // the result says something actually changed). + viewModel.commentChanged.observeEvent(viewLifecycleOwner) { + requireActivity().setResult(AppCompatActivity.RESULT_OK) + } } private fun clearReplyInput() { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsViewModel.kt index f1cff717dadc..22f8d361d34d 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsViewModel.kt @@ -60,11 +60,19 @@ class UnifiedCommentDetailsViewModel @Inject constructor( private val _uiState = MutableLiveData() private val _uiActionEvent = MutableLiveData>() private val _onSnackbarMessage = MutableLiveData>() + private val _commentChanged = MutableLiveData>() val uiState: LiveData = _uiState val uiActionEvent: LiveData> = _uiActionEvent val onSnackbarMessage: LiveData> = _onSnackbarMessage + /** + * Fires when the comment was changed on the server (moderated, replied to, edited or + * deleted) — NOT on like, which the list doesn't render. The host activity reports it as + * its result so the rs list only refreshes when there's something new to show. + */ + val commentChanged: LiveData> = _commentChanged + private var isStarted = false private lateinit var site: SiteModel private var remoteCommentId: Long = 0 @@ -83,10 +91,12 @@ class UnifiedCommentDetailsViewModel @Inject constructor( } /** - * Reloads the comment. Used after returning from the edit screen so any edits are reflected. + * The edit screen reported saved changes: mark the comment as changed and reload it so the + * edits are reflected. */ - fun refreshComment() { + fun onCommentEdited() { if (!isStarted) return + _commentChanged.value = Event(Unit) loadComment() } @@ -100,16 +110,32 @@ class UnifiedCommentDetailsViewModel @Inject constructor( if (!isRefresh) { _uiState.value = CommentDetailsUiState(showProgress = true) } - val (rsComment, cached) = withContext(bgDispatcher) { + val (rsComment, cached, fallbackPostTitle) = withContext(bgDispatcher) { val rs = commentsRsDataSource.getComment(site, remoteCommentId) - val local = commentsStore.getCommentByLocalSiteAndRemoteId(site.id, remoteCommentId).firstOrNull() - rs to local + var local = commentsStore.getCommentByLocalSiteAndRemoteId(site.id, remoteCommentId).firstOrNull() + // Opened from the rs list the FluxC cache may not have this comment at all (the + // legacy list guaranteed a row before the detail could open). Fetch it so the + // post title, like state and the edit screen's local id are available — WP.com + // only, since FluxC has no application-password transport. + if (local == null && site.isUsingWpComRestApi) { + commentsStore.fetchComment(site, remoteCommentId, null) + local = commentsStore.getCommentByLocalSiteAndRemoteId(site.id, remoteCommentId).firstOrNull() + } + // Still no title (self-hosted application-password site, or the fetch failed): + // resolve it the way the rs list does — usually a free hit on the shared title + // cache the list populated moments earlier. + val fallbackTitle = if (local?.postTitle.isNullOrBlank() && rs != null && rs.postId > 0) { + commentsRsDataSource.fetchPostTitles(site, listOf(rs.postId))[rs.postId].orEmpty() + } else { + "" + } + Triple(rs, local, fallbackTitle) } when { rsComment != null -> { loadedComment = rsComment localCommentId = cached?.id?.toInt() ?: 0 - _uiState.value = rsComment.toUiState(cached) + _uiState.value = rsComment.toUiState(cached, fallbackPostTitle) } isRefresh -> showSnackbar(R.string.error_load_comment) else -> _onSnackbarMessage.value = Event( @@ -205,6 +231,7 @@ class UnifiedCommentDetailsViewModel @Inject constructor( if (result is RsResult.Error) { showError(result.message, R.string.error_generic) } else { + _commentChanged.value = Event(Unit) // Replying to an unapproved comment implicitly approves it, matching legacy behaviour if (currentStatus() == UNAPPROVED) { approveAfterReply() @@ -236,8 +263,11 @@ class UnifiedCommentDetailsViewModel @Inject constructor( if (result is RsResult.Error) { _uiState.value = _uiState.value?.copy(status = previousStatus) showError(result.message, R.string.error_moderate_comment) - } else if (closeOnSuccess) { - _uiActionEvent.value = Event(Close) + } else { + _commentChanged.value = Event(Unit) + if (closeOnSuccess) { + _uiActionEvent.value = Event(Close) + } } } } @@ -284,14 +314,14 @@ class UnifiedCommentDetailsViewModel @Inject constructor( _onSnackbarMessage.value = Event(SnackbarMessageHolder(uiMessage)) } - private fun RsComment.toUiState(cached: CommentEntity?) = CommentDetailsUiState( + private fun RsComment.toUiState(cached: CommentEntity?, fallbackPostTitle: String) = CommentDetailsUiState( showProgress = false, contentVisible = true, authorName = authorName, authorAvatarUrl = authorAvatarUrl, datePublished = dateTimeUtilsWrapper.javaDateToTimeSpan(dateGmt), commentText = contentHtml, - postTitle = cached?.postTitle ?: "", + postTitle = cached?.postTitle?.takeIf { it.isNotBlank() } ?: fallbackPostTitle, commentUrl = url, status = status, isLiked = cached?.iLike ?: false diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt index f2a27e564543..971634de3e91 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt @@ -23,12 +23,15 @@ import org.wordpress.android.util.extensions.setContent class CommentsRsListActivity : BaseAppCompatActivity() { private val viewModel: CommentsRsListViewModel by viewModels() - // The detail can moderate, reply, edit or delete, and doesn't report a result — refresh - // unconditionally on return so the list reflects whatever happened there. + // The detail reports RESULT_OK when it changed the comment (moderation, reply, edit, + // delete); only then refresh, so a view-only visit keeps the paged lists and scroll + // position intact. private val detailLauncher = registerForActivityResult( ActivityResultContracts.StartActivityForResult() - ) { - viewModel.refreshAllTabs() + ) { result -> + if (result.resultCode == RESULT_OK) { + viewModel.refreshAllTabs() + } } override fun onCreate(savedInstanceState: Bundle?) { @@ -43,6 +46,7 @@ class CommentsRsListActivity : BaseAppCompatActivity() { tabStates = tabStates, snackbarMessages = viewModel.snackbarMessages, onInitTab = viewModel::initTab, + onTabChanged = viewModel::onTabChanged, onRefreshTab = { tab -> viewModel.refreshTab(tab, isUserRefresh = true) }, onLoadMore = viewModel::loadMore, onNavigateBack = { onBackPressedDispatcher.onBackPressed() }, diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListTab.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListTab.kt index 66978db5abef..fa614ead0885 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListTab.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListTab.kt @@ -18,31 +18,38 @@ import uniffi.wp_api.CommentStatus as RsCommentStatus enum class CommentsRsListTab( @StringRes val labelResId: Int, @StringRes val emptyMessageResId: Int, + /** The `selected_filter` property value for COMMENT_FILTER_CHANGED, matching the legacy list. */ + @StringRes val trackingLabelResId: Int, val queryStatus: RsCommentStatus ) { ALL( R.string.comment_status_all, R.string.comments_empty_list, + R.string.comment_tracker_label_all, RsCommentStatus.Custom("all") ), PENDING( R.string.comment_status_unapproved, R.string.comments_empty_list_filtered_pending, + R.string.comment_tracker_label_pending, RsCommentStatus.Hold ), APPROVED( R.string.comment_status_approved, R.string.comments_empty_list_filtered_approved, + R.string.comment_tracker_label_approved, RsCommentStatus.Custom("approve") ), SPAM( R.string.comment_status_spam, R.string.comments_empty_list_filtered_spam, + R.string.comment_tracker_label_spam, RsCommentStatus.Spam ), TRASHED( R.string.comment_status_trash, R.string.comments_empty_list_filtered_trashed, + R.string.comment_tracker_label_trashed, RsCommentStatus.Trash ) } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt index 8b436a71238d..387262fcd218 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt @@ -14,10 +14,11 @@ import kotlinx.coroutines.flow.receiveAsFlow import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import org.wordpress.android.R +import org.wordpress.android.analytics.AnalyticsTracker.Stat import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.modules.BG_THREAD import org.wordpress.android.ui.comments.unified.CommentsRsDataSource -import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsCommentListItem +import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsComment import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsCommentsPageResult import org.wordpress.android.ui.mysite.SelectedSiteRepository import org.wordpress.android.ui.postsrs.PostRsErrorUtils @@ -26,6 +27,7 @@ import org.wordpress.android.util.DateTimeUtilsWrapper import org.wordpress.android.util.HtmlUtils import org.wordpress.android.util.NetworkUtilsWrapper import org.wordpress.android.util.WPAvatarUtilsWrapper +import org.wordpress.android.util.analytics.AnalyticsTrackerWrapper import org.wordpress.android.viewmodel.ResourceProvider import uniffi.wp_api.CommentListParams import javax.inject.Inject @@ -44,6 +46,7 @@ class CommentsRsListViewModel @Inject constructor( private val networkUtilsWrapper: NetworkUtilsWrapper, private val dateTimeUtilsWrapper: DateTimeUtilsWrapper, private val avatarUtilsWrapper: WPAvatarUtilsWrapper, + private val analyticsTracker: AnalyticsTrackerWrapper, @Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher ) : ViewModel() { private val _tabStates = MutableStateFlow>(emptyMap()) @@ -61,7 +64,11 @@ class CommentsRsListViewModel @Inject constructor( // Bumped whenever a first page is applied. A load-more result whose fetch started before the // bump paged from a cursor that predates the new page 1, so it's stale and gets discarded. private val pageGenerations = mutableMapOf() - private val initializingTabs = mutableSetOf() + + // The in-flight first-page fetch per tab, so overlapping refreshes don't duplicate it. + private val firstPageJobs = mutableMapOf() + + private var lastTrackedTab: CommentsRsListTab? = null // The in-flight title-resolve job per tab, with the ids it's resolving so an identical // request isn't cancelled and re-fired when an applied page adds nothing new. @@ -78,13 +85,14 @@ class CommentsRsListViewModel @Inject constructor( } } - /** Loads the first page for [tab] unless it's already initialized or initializing. */ + /** Loads the first page for [tab] unless it's already initialized. */ @MainThread fun initTab(tab: CommentsRsListTab) { - if (_tabStates.value.containsKey(tab) || initializingTabs.contains(tab)) return - initializingTabs.add(tab) + // updateTabUiState inserts the tab synchronously, so this also blocks re-entry while + // the first fetch is in flight. + if (_tabStates.value.containsKey(tab)) return updateTabUiState(tab) { copy(isLoading = true) } - fetchFirstPage(tab) { initializingTabs.remove(tab) } + fetchFirstPage(tab) } /** @@ -95,17 +103,21 @@ class CommentsRsListViewModel @Inject constructor( @MainThread fun refreshTab(tab: CommentsRsListTab, isUserRefresh: Boolean = false) { 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. + if (firstPageJobs[tab]?.isActive == true) return if (isUserRefresh) { - // An explicit refresh also retries post titles that previously resolved as - // unresolvable — e.g. a post that has been published since it was first seen. - commentsRsDataSource.clearUnresolvedPostTitles(site) - // A resolve job that started before the clear may apply pre-clear "not found" - // titles; cancel it so the post-refresh pass fetches fresh results instead of - // deferring to it via the identical-ids guard. + // An explicit refresh retries every post title for the site: ones that previously + // resolved as unresolvable (e.g. a post published since it was first seen) and ones + // that may have gone stale (e.g. a post renamed since it was cached). + commentsRsDataSource.clearPostTitles(site) + // A resolve job that started before the clear may apply pre-clear titles; cancel it + // so the post-refresh pass fetches fresh results instead of deferring to it via the + // identical-ids guard. resolveTitleJobs[tab]?.first?.cancel() } updateTabUiState(tab) { copy(isRefreshing = isUserRefresh, error = null) } - fetchFirstPage(tab) + fetchFirstPage(tab, showErrorSnackbar = isUserRefresh) } /** Refreshes every initialized tab; comments move between tabs when moderated. */ @@ -172,15 +184,28 @@ class CommentsRsListViewModel @Inject constructor( _events.trySend(CommentsRsListEvent.OpenCommentDetail(site, remoteCommentId)) } - private fun fetchFirstPage(tab: CommentsRsListTab, onComplete: () -> Unit = {}) { - viewModelScope.launch { + /** + * Tracks the tab the pager settled on — the same COMMENT_FILTER_CHANGED event (including the + * initial selection) the legacy list emits, so filter metrics stay comparable. + */ + @MainThread + fun onTabChanged(tab: CommentsRsListTab) { + if (tab == lastTrackedTab) return + lastTrackedTab = tab + analyticsTracker.track( + Stat.COMMENT_FILTER_CHANGED, + mapOf(SELECTED_FILTER_PROPERTY to resourceProvider.getString(tab.trackingLabelResId)) + ) + } + + private fun fetchFirstPage(tab: CommentsRsListTab, showErrorSnackbar: Boolean = true) { + firstPageJobs[tab] = viewModelScope.launch { val params = commentsRsDataSource.firstPageParams(status = tab.queryStatus) val result = withContext(bgDispatcher) { commentsRsDataSource.fetchCommentsPage(site, params) } when (result) { is RsCommentsPageResult.Success -> applyPage(tab, result, append = false) - is RsCommentsPageResult.Error -> onFirstPageError(tab, result.message) + is RsCommentsPageResult.Error -> onFirstPageError(tab, result.message, showErrorSnackbar) } - onComplete() } } @@ -194,42 +219,42 @@ class CommentsRsListViewModel @Inject constructor( nextPageParams[tab] = result.nextPageParams val newRows = result.comments.map { it.toUiModel() } updateTabUiState(tab) { - if (append) { - copy( - // A comment can shift pages if the list changed server-side between - // requests, so dedupe on append. - comments = (comments + newRows).distinctBy { it.remoteCommentId }, - isLoadingMore = false, - canLoadMore = result.nextPageParams != null - ) - } else { - copy( - comments = newRows, - isLoading = false, - isRefreshing = false, - canLoadMore = result.nextPageParams != null, - error = null - ) - } + copy( + // A comment can shift pages if the list changed server-side between requests, + // so dedupe on append. + comments = if (append) (comments + newRows).distinctBy { it.remoteCommentId } else newRows, + isLoading = false, + isRefreshing = false, + // A replacing page must clear isLoadingMore too: a load-more in flight when the + // refresh landed is discarded by the generation check, so nothing else resets + // the flag promptly and paging would stay blocked behind the isBusy guard. + isLoadingMore = false, + canLoadMore = result.nextPageParams != null, + error = null + ) } resolvePostTitles(tab) } /** * First-page failure: when the tab already shows comments keep them and offer a retry - * snackbar; when it's empty, show the full-screen error state. + * snackbar; when it's empty, show the full-screen error state. Silent refreshes (returning + * from the detail) skip the snackbar — a failed background refresh shouldn't interrupt a + * user who's still looking at a perfectly good list. */ - private fun onFirstPageError(tab: CommentsRsListTab, message: String?) { + private fun onFirstPageError(tab: CommentsRsListTab, message: String?, showErrorSnackbar: Boolean) { val friendly = errorMessage(message) if (getTabUiState(tab).comments.isNotEmpty()) { updateTabUiState(tab) { copy(isLoading = false, isRefreshing = false, error = null) } - _snackbarMessages.trySend( - SnackbarMessage( - message = friendly, - actionLabel = resourceProvider.getString(R.string.retry), - onAction = { refreshTab(tab) } + if (showErrorSnackbar) { + _snackbarMessages.trySend( + SnackbarMessage( + message = friendly, + actionLabel = resourceProvider.getString(R.string.retry), + onAction = { refreshTab(tab, isUserRefresh = true) } + ) ) - ) + } } else { updateTabUiState(tab) { copy(comments = emptyList(), isLoading = false, isRefreshing = false, error = friendly) @@ -269,7 +294,7 @@ class CommentsRsListViewModel @Inject constructor( serverMessage?.takeIf { it.isNotBlank() } ?: PostRsErrorUtils.friendlyErrorMessage(null, null, resourceProvider, networkUtilsWrapper) - private fun RsCommentListItem.toUiModel() = CommentRsUiModel( + private fun RsComment.toUiModel() = CommentRsUiModel( remoteCommentId = remoteCommentId, authorName = authorName.ifBlank { resourceProvider.getString(R.string.anonymous) }, avatarUrl = avatarUtilsWrapper.rewriteAvatarUrlWithResource(authorAvatarUrl, R.dimen.avatar_sz_medium), @@ -290,5 +315,8 @@ class CommentsRsListViewModel @Inject constructor( // How many extra pages a single load-more may fetch unattended when applied pages // add no rows, before waiting for the next user scroll. private const val MAX_AUTO_ADVANCE_PAGES = 3 + + // Same property key as the legacy list's tracking (UnifiedCommentsActivity). + private const val SELECTED_FILTER_PROPERTY = "selected_filter" } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt index ce6b05bc0c28..037f0273b05f 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt @@ -41,6 +41,7 @@ fun CommentsRsListScreen( tabStates: Map, snackbarMessages: Flow = emptyFlow(), onInitTab: (CommentsRsListTab) -> Unit, + onTabChanged: (CommentsRsListTab) -> Unit, onRefreshTab: (CommentsRsListTab) -> Unit, onLoadMore: (CommentsRsListTab) -> Unit, onNavigateBack: () -> Unit, @@ -99,6 +100,7 @@ fun CommentsRsListScreen( LaunchedEffect(pagerState) { snapshotFlow { pagerState.settledPage }.collect { page -> onInitTab(tabs[page]) + onTabChanged(tabs[page]) } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt index 89f118963254..ea83ebaf4806 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt @@ -95,12 +95,17 @@ private fun CommentListContent( ) { val listState = rememberLazyListState() - LaunchedEffect(canLoadMore) { + // Also keyed on the list size: a refresh that truncates the list (or an appended page) restarts + // the flow, so a `true` latched by distinctUntilChanged before the change can't suppress the + // re-fire needed to resume paging. The ViewModel's busy/cursor guards make re-fires safe. + LaunchedEffect(canLoadMore, comments.size) { if (!canLoadMore) return@LaunchedEffect snapshotFlow { + // total > 0 keeps the pre-layout pass (empty layoutInfo, 0 >= -threshold) from + // triggering a load of the next page before the user has scrolled at all. val lastVisible = listState.layoutInfo.visibleItemsInfo.lastOrNull()?.index ?: 0 val total = listState.layoutInfo.totalItemsCount - lastVisible >= total - LOAD_MORE_THRESHOLD + total > 0 && lastVisible >= total - LOAD_MORE_THRESHOLD }.distinctUntilChanged().collect { shouldLoad -> if (shouldLoad) onLoadMore() } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt deleted file mode 100644 index a313fa3bbe52..000000000000 --- a/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsDataSourceTest.kt +++ /dev/null @@ -1,253 +0,0 @@ -package org.wordpress.android.ui.comments.unified - -import kotlinx.coroutines.test.runTest -import org.assertj.core.api.Assertions.assertThat -import org.junit.Before -import org.junit.Test -import org.mockito.kotlin.any -import org.mockito.kotlin.anyOrNull -import org.mockito.kotlin.doSuspendableAnswer -import org.mockito.kotlin.mock -import org.mockito.kotlin.stub -import org.mockito.kotlin.whenever -import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider -import rs.wordpress.api.kotlin.WpApiClient -import rs.wordpress.api.kotlin.WpRequestResult -import uniffi.wp_api.PostEndpointType -import uniffi.wp_api.PostListParams -import uniffi.wp_api.PostsRequestExecutor -import uniffi.wp_api.PostsRequestFilterListWithViewContextResponse -import uniffi.wp_api.RequestMethod -import uniffi.wp_api.SparseAnyPostWithViewContext -import uniffi.wp_api.SparsePostTitleWithViewContext -import uniffi.wp_api.UniffiWpApiClient -import uniffi.wp_api.WpErrorCode - -/** - * Tests for [CommentsRsDataSource.fetchPostTitles]: the per-site title cache, the posts→pages - * endpoint fallback, negative caching of unresolvable ids, and request chunking. - * - * The [wpApiClient] stub executes each request's builder lambda against a mocked - * [UniffiWpApiClient], recording the endpoint and paging params actually sent — so the tests - * pin the requests made, not just how many there were. - */ -class CommentsRsDataSourceTest { - private data class RecordedRequest( - val endpointType: PostEndpointType, - val includeCount: Int, - val perPage: UInt? - ) - - private val wpApiClientProvider: WpApiClientProvider = mock() - private val wpApiClient: WpApiClient = mock() - private val uniffiClient: UniffiWpApiClient = mock() - private val postsExecutor: PostsRequestExecutor = mock() - private lateinit var dataSource: CommentsRsDataSource - - private val recordedRequests = mutableListOf() - private val cannedResults = ArrayDeque>() - - /** Invoked after each request completes; lets a test simulate concurrent work mid-fetch. */ - private var afterRequest: (() -> Unit)? = null - - private val siteA = SiteModel().apply { id = 1 } - private val siteB = SiteModel().apply { id = 2 } - - @Before - fun setUp() { - dataSource = CommentsRsDataSource(wpApiClientProvider) - whenever(wpApiClientProvider.getWpApiClient(any(), anyOrNull())).thenReturn(wpApiClient) - whenever(uniffiClient.posts()).thenReturn(postsExecutor) - postsExecutor.stub { - on { filterListWithViewContext(any(), any(), any()) } doSuspendableAnswer { invocation -> - val params = invocation.getArgument(1) - recordedRequests += RecordedRequest( - endpointType = invocation.getArgument(0), - includeCount = params.include.size, - perPage = params.perPage - ) - // The payload is decided by the request-level stub below; this value is unused. - PostsRequestFilterListWithViewContextResponse(emptyList(), mock(), null, null) - } - } - wpApiClient.stub { - on { request(any()) } doSuspendableAnswer { invocation -> - // Run the builder lambda so the executor stub above records what was requested. - val executor = invocation.getArgument Any>(0) - executor(uniffiClient) - val result = cannedResults.removeFirst() - afterRequest?.invoke() - result - } - } - } - - @Test - fun `resolved titles are cached and skip the network`() = runTest { - stubRequests(successResponse(sparsePost(5, "Hello"))) - - assertThat(dataSource.fetchPostTitles(siteA, listOf(5))).isEqualTo(mapOf(5L to "Hello")) - assertThat(dataSource.fetchPostTitles(siteA, listOf(5))).isEqualTo(mapOf(5L to "Hello")) - - assertThat(recordedRequests).hasSize(1) - } - - @Test - fun `titles are cached per site, not just per post id`() = runTest { - stubRequests( - successResponse(sparsePost(5, "Site A post")), - successResponse(sparsePost(5, "Site B post")) - ) - - assertThat(dataSource.fetchPostTitles(siteA, listOf(5))).isEqualTo(mapOf(5L to "Site A post")) - assertThat(dataSource.fetchPostTitles(siteB, listOf(5))).isEqualTo(mapOf(5L to "Site B post")) - - assertThat(recordedRequests).hasSize(2) - } - - @Test - fun `a posts endpoint failure still tries the pages endpoint`() = runTest { - stubRequests(wpError(), successResponse(sparsePost(7, "About"))) - - assertThat(dataSource.fetchPostTitles(siteA, listOf(7))).isEqualTo(mapOf(7L to "About")) - - assertThat(recordedRequests.map { it.endpointType }) - .containsExactly(PostEndpointType.Posts, PostEndpointType.Pages) - } - - @Test - fun `ids neither endpoint returns are negative-cached once both succeed`() = runTest { - stubRequests(successResponse(), successResponse()) - - assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "")) - assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "")) - - // Posts + pages for the first call, none for the second. - assertThat(recordedRequests.map { it.endpointType }) - .containsExactly(PostEndpointType.Posts, PostEndpointType.Pages) - } - - @Test - fun `a transient failure is not negative-cached`() = runTest { - stubRequests( - successResponse(), // posts: id not found - wpError(), // pages: transient failure — must NOT negative-cache - successResponse(sparsePost(9, "Resolved later")) - ) - - assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEmpty() - assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "Resolved later")) - - assertThat(recordedRequests.map { it.endpointType }) - .containsExactly(PostEndpointType.Posts, PostEndpointType.Pages, PostEndpointType.Posts) - } - - @Test - fun `clearUnresolvedPostTitles retries negative-cached ids`() = runTest { - stubRequests( - successResponse(), // posts: not found - successResponse(), // pages: not found → negative-cached - successResponse(sparsePost(9, "Now published")) - ) - assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "")) - - dataSource.clearUnresolvedPostTitles(siteA) - - assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "Now published")) - assertThat(recordedRequests).hasSize(3) - } - - @Test - fun `a clear during an in-flight fetch prevents negative caching`() = runTest { - stubRequests( - successResponse(), // posts: not found - successResponse(), // pages: not found — but a clear lands before the cache write - successResponse(), // retry: posts - successResponse() // retry: pages - ) - afterRequest = { - // Simulates another surface clearing while this fetch is between its network - // responses and its negative-cache write. - if (recordedRequests.size == 2) dataSource.clearUnresolvedPostTitles(siteA) - } - - assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEmpty() - - // Not negative-cached, so a later fetch goes back to the network. - afterRequest = null - assertThat(dataSource.fetchPostTitles(siteA, listOf(9))).isEqualTo(mapOf(9L to "")) - assertThat(recordedRequests).hasSize(4) - } - - @Test - fun `batches over 100 ids are chunked to the per_page maximum`() = runTest { - val ids = (1L..101L).toList() - stubRequests( - successResponse(*ids.take(100).map { sparsePost(it, "Post $it") }.toTypedArray()), - successResponse(sparsePost(101, "Post 101")) - ) - - val titles = dataSource.fetchPostTitles(siteA, ids) - - assertThat(titles).hasSize(101) - assertThat(titles[101L]).isEqualTo("Post 101") - // Two posts-endpoint requests with the batch split at 100, each with a matching - // explicit perPage — NOT one oversized request plus a pages fallback. - assertThat(recordedRequests.map { Triple(it.endpointType, it.includeCount, it.perPage) }) - .containsExactly( - Triple(PostEndpointType.Posts, 100, 100u), - Triple(PostEndpointType.Posts, 1, 1u) - ) - } - - // A sparse post as returned by the id+title sparse-field request: everything else null. - private fun sparsePost(id: Long, title: String) = SparseAnyPostWithViewContext( - id = id, - date = null, - dateGmt = null, - guid = null, - link = null, - modified = null, - modifiedGmt = null, - slug = null, - status = null, - postType = null, - title = SparsePostTitleWithViewContext(rendered = title), - content = null, - author = null, - excerpt = null, - featuredMedia = null, - commentStatus = null, - pingStatus = null, - format = null, - meta = null, - sticky = null, - template = null, - categories = null, - tags = null, - parent = null, - menuOrder = null, - additionalFields = null - ) - - private fun successResponse(vararg posts: SparseAnyPostWithViewContext) = WpRequestResult.Success( - response = PostsRequestFilterListWithViewContextResponse(posts.toList(), mock(), null, null) - ) - - private fun wpError() = WpRequestResult.WpError( - errorCode = WpErrorCode.Forbidden(), - errorMessage = "server said no", - statusCode = 403u, - response = "", - requestUrl = "https://example.com", - requestMethod = RequestMethod.GET - ) - - @Suppress("UNCHECKED_CAST") - private fun stubRequests(vararg responses: WpRequestResult<*>) { - recordedRequests.clear() - cannedResults.clear() - responses.forEach { cannedResults.add(it as WpRequestResult) } - } -} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsListMappingTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsListMappingTest.kt deleted file mode 100644 index a3ff1a094ffc..000000000000 --- a/WordPress/src/test/java/org/wordpress/android/ui/comments/unified/CommentsRsListMappingTest.kt +++ /dev/null @@ -1,110 +0,0 @@ -package org.wordpress.android.ui.comments.unified - -import org.assertj.core.api.Assertions.assertThat -import org.junit.Test -import org.mockito.kotlin.mock -import org.wordpress.android.fluxc.model.CommentStatus.APPROVED -import org.wordpress.android.fluxc.model.CommentStatus.UNAPPROVED -import uniffi.wp_api.CommentContentWithViewContext -import uniffi.wp_api.CommentType -import uniffi.wp_api.CommentWithViewContext -import uniffi.wp_api.UserAvatarSize -import uniffi.wp_api.WpAdditionalFields -import uniffi.wp_api.WpApiParamCommentsOrderBy -import uniffi.wp_api.WpApiParamOrder -import java.util.Date -import uniffi.wp_api.CommentStatus as RsCommentStatus - -class CommentsRsListMappingTest { - @Test - fun `toRsCommentListItem maps the fields the list rows need`() { - val item = rsComment().toRsCommentListItem() - - assertThat(item.remoteCommentId).isEqualTo(COMMENT_ID) - assertThat(item.authorName).isEqualTo("Jane") - assertThat(item.authorAvatarUrl).isEqualTo("https://example.com/avatar96.png") - assertThat(item.dateGmt).isEqualTo(DATE_GMT) - assertThat(item.contentHtml).isEqualTo("

hello

") - assertThat(item.postId).isEqualTo(POST_ID) - assertThat(item.status).isEqualTo(APPROVED) - } - - @Test - fun `toRsCommentListItem maps hold status to unapproved`() { - val item = rsComment(status = RsCommentStatus.Hold).toRsCommentListItem() - - assertThat(item.status).isEqualTo(UNAPPROVED) - } - - @Test - fun `pickAvatarUrl prefers size 96`() { - val comment = rsComment( - avatarUrls = mapOf( - UserAvatarSize.Size24 to "https://example.com/avatar24.png", - UserAvatarSize.Size96 to "https://example.com/avatar96.png" - ) - ) - - assertThat(comment.pickAvatarUrl()).isEqualTo("https://example.com/avatar96.png") - } - - @Test - fun `pickAvatarUrl falls back to the first non-empty url`() { - val comment = rsComment( - avatarUrls = mapOf( - UserAvatarSize.Size24 to "", - UserAvatarSize.Size48 to "https://example.com/avatar48.png" - ) - ) - - assertThat(comment.pickAvatarUrl()).isEqualTo("https://example.com/avatar48.png") - } - - @Test - fun `pickAvatarUrl returns empty when there are no avatar urls`() { - assertThat(rsComment(avatarUrls = emptyMap()).pickAvatarUrl()).isEmpty() - } - - @Test - fun `firstPageParams requests newest comments with the given status and search`() { - val dataSource = CommentsRsDataSource(mock()) - - val params = dataSource.firstPageParams(RsCommentStatus.Spam, search = "query") - - assertThat(params.perPage).isEqualTo(CommentsRsDataSource.COMMENTS_PAGE_SIZE) - assertThat(params.status).isEqualTo(RsCommentStatus.Spam) - assertThat(params.search).isEqualTo("query") - assertThat(params.orderby).isEqualTo(WpApiParamCommentsOrderBy.DATE_GMT) - assertThat(params.order).isEqualTo(WpApiParamOrder.DESC) - } - - private fun rsComment( - status: RsCommentStatus = RsCommentStatus.Approved, - avatarUrls: Map = mapOf( - UserAvatarSize.Size96 to "https://example.com/avatar96.png" - ) - ) = CommentWithViewContext( - id = COMMENT_ID, - author = 7L, - authorName = "Jane", - authorUrl = "https://example.com", - content = CommentContentWithViewContext(rendered = "

hello

"), - date = "2026-07-01T12:00:00", - dateGmt = DATE_GMT, - link = "https://example.com/post/#comment-42", - parent = 0L, - post = POST_ID, - status = status, - commentType = CommentType.Comment, - authorAvatarUrls = avatarUrls, - // Mocked: the real WpAdditionalFields constructor loads the uniffi native library, - // which isn't available in local unit tests. - additionalFields = mock() - ) - - companion object { - private const val COMMENT_ID = 42L - private const val POST_ID = 99L - private val DATE_GMT = Date(1_700_000_000_000) - } -} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/comments/viewmodels/UnifiedCommentDetailsViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/comments/viewmodels/UnifiedCommentDetailsViewModelTest.kt index 30aa5d19eb25..9880c54ce1d1 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/comments/viewmodels/UnifiedCommentDetailsViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/comments/viewmodels/UnifiedCommentDetailsViewModelTest.kt @@ -120,7 +120,7 @@ class UnifiedCommentDetailsViewModelTest : BaseUnitTest() { viewModel.start(site, REMOTE_COMMENT_ID) whenever(commentsRsDataSource.getComment(site, REMOTE_COMMENT_ID)).thenReturn(null) - viewModel.refreshComment() + viewModel.onCommentEdited() assertThat(uiStates.last().contentVisible).isTrue assertThat(uiStates.last().status).isEqualTo(APPROVED) @@ -135,7 +135,7 @@ class UnifiedCommentDetailsViewModelTest : BaseUnitTest() { delay(LOAD_DELAY_MS) RS_COMMENT } - viewModel.refreshComment() + viewModel.onCommentEdited() viewModel.onApproveClicked() advanceUntilIdle() @@ -376,6 +376,7 @@ class UnifiedCommentDetailsViewModelTest : BaseUnitTest() { private const val LOAD_DELAY_MS = 1000L private val RS_COMMENT = RsComment( + remoteCommentId = REMOTE_COMMENT_ID, authorName = "authorName", authorAvatarUrl = "", dateGmt = Date(0), diff --git a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListTabTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListTabTest.kt deleted file mode 100644 index 023bff617408..000000000000 --- a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListTabTest.kt +++ /dev/null @@ -1,26 +0,0 @@ -package org.wordpress.android.ui.commentsrs - -import org.assertj.core.api.Assertions.assertThat -import org.junit.Test -import uniffi.wp_api.CommentStatus as RsCommentStatus - -class CommentsRsListTabTest { - @Test - fun `all tab queries status all which the server treats as approved plus hold`() { - assertThat(CommentsRsListTab.ALL.queryStatus).isEqualTo(RsCommentStatus.Custom("all")) - } - - @Test - fun `approved tab queries the literal approve status`() { - // WP_Comment_Query only recognises "approve"; the RsCommentStatus.Approved enum - // serialises to "approved", which the server treats as unknown and returns nothing for. - assertThat(CommentsRsListTab.APPROVED.queryStatus).isEqualTo(RsCommentStatus.Custom("approve")) - } - - @Test - fun `remaining tabs use the built-in statuses`() { - assertThat(CommentsRsListTab.PENDING.queryStatus).isEqualTo(RsCommentStatus.Hold) - assertThat(CommentsRsListTab.SPAM.queryStatus).isEqualTo(RsCommentStatus.Spam) - assertThat(CommentsRsListTab.TRASHED.queryStatus).isEqualTo(RsCommentStatus.Trash) - } -} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt deleted file mode 100644 index dbc2d9fe4fa2..000000000000 --- a/WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModelTest.kt +++ /dev/null @@ -1,414 +0,0 @@ -package org.wordpress.android.ui.commentsrs - -import androidx.lifecycle.viewModelScope -import app.cash.turbine.test -import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.cancel -import kotlinx.coroutines.delay -import kotlinx.coroutines.test.StandardTestDispatcher -import kotlinx.coroutines.test.runCurrent -import org.assertj.core.api.Assertions.assertThat -import org.junit.After -import org.junit.Before -import org.junit.Test -import org.mockito.Mock -import org.mockito.kotlin.any -import org.mockito.kotlin.anyOrNull -import org.mockito.kotlin.doSuspendableAnswer -import org.mockito.kotlin.eq -import org.mockito.kotlin.never -import org.mockito.kotlin.times -import org.mockito.kotlin.verify -import org.mockito.kotlin.whenever -import org.wordpress.android.BaseUnitTest -import org.wordpress.android.R -import org.wordpress.android.fluxc.model.CommentStatus.APPROVED -import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.ui.comments.unified.CommentsRsDataSource -import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsCommentListItem -import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsCommentsPageResult -import org.wordpress.android.ui.mysite.SelectedSiteRepository -import org.wordpress.android.util.DateTimeUtilsWrapper -import org.wordpress.android.util.NetworkUtilsWrapper -import org.wordpress.android.util.WPAvatarUtilsWrapper -import org.wordpress.android.viewmodel.ResourceProvider -import uniffi.wp_api.CommentListParams -import java.util.Date - -@ExperimentalCoroutinesApi -class CommentsRsListViewModelTest : BaseUnitTest(StandardTestDispatcher()) { - @Mock lateinit var selectedSiteRepository: SelectedSiteRepository - @Mock lateinit var commentsRsDataSource: CommentsRsDataSource - @Mock lateinit var resourceProvider: ResourceProvider - @Mock lateinit var networkUtilsWrapper: NetworkUtilsWrapper - @Mock lateinit var dateTimeUtilsWrapper: DateTimeUtilsWrapper - @Mock lateinit var avatarUtilsWrapper: WPAvatarUtilsWrapper - - private lateinit var site: SiteModel - private var activeViewModel: CommentsRsListViewModel? = null - - @Before - fun setUp() = test { - site = SiteModel().apply { - id = 1 - siteId = 123L - } - whenever(selectedSiteRepository.getSelectedSite()).thenReturn(site) - whenever(resourceProvider.getString(any())).thenReturn("string") - whenever(dateTimeUtilsWrapper.javaDateToTimeSpan(any())).thenReturn("2 hours ago") - whenever(avatarUtilsWrapper.rewriteAvatarUrlWithResource(any(), any())).thenAnswer { it.arguments[0] } - whenever(commentsRsDataSource.firstPageParams(any(), anyOrNull())).thenReturn(FIRST_PAGE) - whenever(commentsRsDataSource.fetchPostTitles(any(), any())).thenReturn(emptyMap()) - } - - @After - fun tearDown() { - activeViewModel?.viewModelScope?.cancel() - activeViewModel = null - } - - private fun createViewModel() = CommentsRsListViewModel( - selectedSiteRepository = selectedSiteRepository, - commentsRsDataSource = commentsRsDataSource, - resourceProvider = resourceProvider, - networkUtilsWrapper = networkUtilsWrapper, - dateTimeUtilsWrapper = dateTimeUtilsWrapper, - avatarUtilsWrapper = avatarUtilsWrapper, - bgDispatcher = testDispatcher() - ).also { activeViewModel = it } - - private suspend fun givenPage( - comments: List, - nextPageParams: CommentListParams? = null - ) { - whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())) - .thenReturn(RsCommentsPageResult.Success(comments, nextPageParams)) - } - - @Test - fun `when no site selected, emits ShowToast and Finish`() = test { - whenever(selectedSiteRepository.getSelectedSite()).thenReturn(null) - - val viewModel = createViewModel() - - viewModel.events.test { - val first = awaitItem() - assertThat(first).isInstanceOf(CommentsRsListEvent.ShowToast::class.java) - assertThat((first as CommentsRsListEvent.ShowToast).messageResId).isEqualTo(R.string.blog_not_found) - assertThat(awaitItem()).isEqualTo(CommentsRsListEvent.Finish) - cancelAndIgnoreRemainingEvents() - } - } - - @Test - fun `initTab loads the first page and maps rows`() = test { - givenPage(listOf(rsItem(id = 1), rsItem(id = 2)), nextPageParams = NEXT_PAGE) - val viewModel = createViewModel() - - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - - val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) - assertThat(state.isLoading).isFalse() - assertThat(state.comments).hasSize(2) - assertThat(state.comments.first().remoteCommentId).isEqualTo(1) - assertThat(state.comments.first().authorName).isEqualTo("Jane") - assertThat(state.comments.first().snippet).isEqualTo("hello") - assertThat(state.canLoadMore).isTrue() - } - - @Test - fun `initTab passes the tab's query status to the data source`() = test { - givenPage(emptyList()) - val viewModel = createViewModel() - - viewModel.initTab(CommentsRsListTab.APPROVED) - advanceUntilIdle() - - verify(commentsRsDataSource).firstPageParams(eq(CommentsRsListTab.APPROVED.queryStatus), anyOrNull()) - } - - @Test - fun `initTab is a no-op when the tab is already initialized`() = test { - givenPage(emptyList()) - val viewModel = createViewModel() - - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - - verify(commentsRsDataSource, times(1)).fetchCommentsPage(eq(site), any()) - } - - @Test - fun `initTab failure with no content shows the error state`() = test { - whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())) - .thenReturn(RsCommentsPageResult.Error("server said no")) - val viewModel = createViewModel() - - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - - val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) - assertThat(state.error).isEqualTo("server said no") - assertThat(state.isLoading).isFalse() - } - - @Test - fun `loadMore appends the next page and dedupes by comment id`() = test { - givenPage(listOf(rsItem(id = 1), rsItem(id = 2)), nextPageParams = NEXT_PAGE) - val viewModel = createViewModel() - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - givenPage(listOf(rsItem(id = 2), rsItem(id = 3)), nextPageParams = null) - - viewModel.loadMore(CommentsRsListTab.ALL) - advanceUntilIdle() - - val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) - assertThat(state.comments.map { it.remoteCommentId }).containsExactly(1L, 2L, 3L) - assertThat(state.canLoadMore).isFalse() - } - - @Test - fun `stale loadMore result arriving after a silent refresh is discarded`() = test { - givenPage(listOf(rsItem(id = 1), rsItem(id = 2)), nextPageParams = NEXT_PAGE) - val viewModel = createViewModel() - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - // Next two fetches: the silent refresh gets a fresh first page; the concurrently - // launched loadMore gets the (now stale) old second page. - whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())).thenReturn( - RsCommentsPageResult.Success(listOf(rsItem(id = 10), rsItem(id = 11)), NEXT_PAGE), - RsCommentsPageResult.Success(listOf(rsItem(id = 3), rsItem(id = 4)), null) - ) - - viewModel.refreshTab(CommentsRsListTab.ALL) // silent: sets no busy flag - viewModel.loadMore(CommentsRsListTab.ALL) - advanceUntilIdle() - - val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) - assertThat(state.comments.map { it.remoteCommentId }).containsExactly(10L, 11L) - assertThat(state.isLoadingMore).isFalse() - assertThat(state.canLoadMore).isTrue() - } - - @Test - fun `loadMore failure offers a retry snackbar`() = test { - givenPage(listOf(rsItem(id = 1)), nextPageParams = NEXT_PAGE) - val viewModel = createViewModel() - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())) - .thenReturn(RsCommentsPageResult.Error("boom")) - - viewModel.snackbarMessages.test { - viewModel.loadMore(CommentsRsListTab.ALL) - advanceUntilIdle() - - val snackbar = awaitItem() - assertThat(snackbar.message).isEqualTo("boom") - assertThat(snackbar.onAction != null).isTrue() - cancelAndIgnoreRemainingEvents() - } - assertThat(viewModel.tabStates.value.getValue(CommentsRsListTab.ALL).isLoadingMore).isFalse() - } - - @Test - fun `fully deduplicated page auto-advances to the next page`() = test { - givenPage(listOf(rsItem(id = 1), rsItem(id = 2)), nextPageParams = NEXT_PAGE) - val viewModel = createViewModel() - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - // The next page duplicates the current rows entirely (server-side shift), then the - // page after that has the genuinely new comment. - whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())).thenReturn( - RsCommentsPageResult.Success(listOf(rsItem(id = 1), rsItem(id = 2)), THIRD_PAGE), - RsCommentsPageResult.Success(listOf(rsItem(id = 3)), null) - ) - - viewModel.loadMore(CommentsRsListTab.ALL) - advanceUntilIdle() - - val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) - assertThat(state.comments.map { it.remoteCommentId }).containsExactly(1L, 2L, 3L) - assertThat(state.canLoadMore).isFalse() - } - - @Test - fun `an empty page with a remaining cursor auto-advances to the next page`() = test { - givenPage(listOf(rsItem(id = 1), rsItem(id = 2)), nextPageParams = NEXT_PAGE) - val viewModel = createViewModel() - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - // A middle page can come back empty (comments deleted server-side) while the - // headers still advertise more pages. - whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())).thenReturn( - RsCommentsPageResult.Success(emptyList(), THIRD_PAGE), - RsCommentsPageResult.Success(listOf(rsItem(id = 3)), null) - ) - - viewModel.loadMore(CommentsRsListTab.ALL) - advanceUntilIdle() - - val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) - assertThat(state.comments.map { it.remoteCommentId }).containsExactly(1L, 2L, 3L) - assertThat(state.canLoadMore).isFalse() - } - - @Test - fun `auto-advance is capped when pages keep adding nothing`() = test { - givenPage(listOf(rsItem(id = 1)), nextPageParams = NEXT_PAGE) - val viewModel = createViewModel() - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - // Pathological cursor chain: every page dedupes away with a cursor still present. - whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())) - .thenReturn(RsCommentsPageResult.Success(listOf(rsItem(id = 1)), NEXT_PAGE)) - - viewModel.loadMore(CommentsRsListTab.ALL) - advanceUntilIdle() - - // 1 init + 1 user loadMore + at most 3 auto-advances, then the chain stops. - verify(commentsRsDataSource, times(5)).fetchCommentsPage(eq(site), any()) - val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) - assertThat(state.isLoadingMore).isFalse() - } - - @Test - fun `a user refresh retries unresolvable post titles, a silent refresh does not`() = test { - givenPage(listOf(rsItem(id = 1))) - val viewModel = createViewModel() - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - - viewModel.refreshTab(CommentsRsListTab.ALL) // silent - advanceUntilIdle() - verify(commentsRsDataSource, never()).clearUnresolvedPostTitles(any()) - - viewModel.refreshTab(CommentsRsListTab.ALL, isUserRefresh = true) - advanceUntilIdle() - verify(commentsRsDataSource).clearUnresolvedPostTitles(site) - } - - @Test - fun `a user refresh cancels an in-flight title resolve and fetches fresh titles`() = test { - givenPage(listOf(rsItem(id = 1, postId = 10))) - // The first resolve hangs mid-flight (holding pre-refresh results); the retry after - // the user refresh returns the fresh title. - whenever(commentsRsDataSource.fetchPostTitles(eq(site), any())) - .doSuspendableAnswer { delay(60_000); emptyMap() } - .thenReturn(mapOf(10L to "Fresh title")) - val viewModel = createViewModel() - viewModel.initTab(CommentsRsListTab.ALL) - runCurrent() // page applied; resolve job now suspended mid-flight - - viewModel.refreshTab(CommentsRsListTab.ALL, isUserRefresh = true) - advanceUntilIdle() - - // Without the cancel, the identical-ids guard defers to the hung pre-refresh job and - // the fresh title is never fetched. - val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) - assertThat(state.comments.first().postTitle).isEqualTo("Fresh title") - verify(commentsRsDataSource, times(2)).fetchPostTitles(eq(site), any()) - } - - @Test - fun `loadMore is a no-op when there is no next page`() = test { - givenPage(listOf(rsItem(id = 1)), nextPageParams = null) - val viewModel = createViewModel() - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - - viewModel.loadMore(CommentsRsListTab.ALL) - advanceUntilIdle() - - verify(commentsRsDataSource, times(1)).fetchCommentsPage(eq(site), any()) - } - - @Test - fun `refresh failure keeps the current comments and offers retry`() = test { - givenPage(listOf(rsItem(id = 1))) - val viewModel = createViewModel() - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - whenever(commentsRsDataSource.fetchCommentsPage(eq(site), any())) - .thenReturn(RsCommentsPageResult.Error("boom")) - - viewModel.snackbarMessages.test { - viewModel.refreshTab(CommentsRsListTab.ALL, isUserRefresh = true) - advanceUntilIdle() - - val snackbar = awaitItem() - assertThat(snackbar.message).isEqualTo("boom") - assertThat(snackbar.onAction != null).isTrue() - cancelAndIgnoreRemainingEvents() - } - val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) - assertThat(state.comments).hasSize(1) - assertThat(state.error).isNull() - } - - @Test - fun `refreshAllTabs refreshes only initialized tabs`() = test { - givenPage(listOf(rsItem(id = 1))) - val viewModel = createViewModel() - viewModel.initTab(CommentsRsListTab.ALL) - viewModel.initTab(CommentsRsListTab.SPAM) - advanceUntilIdle() - - viewModel.refreshAllTabs() - advanceUntilIdle() - - // 2 init fetches + 2 refresh fetches, nothing for the 3 uninitialized tabs - verify(commentsRsDataSource, times(4)).fetchCommentsPage(eq(site), any()) - } - - @Test - fun `onCommentClick emits OpenCommentDetail`() = test { - givenPage(listOf(rsItem(id = 42))) - val viewModel = createViewModel() - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - - viewModel.events.test { - viewModel.onCommentClick(42L) - val event = awaitItem() - assertThat(event).isInstanceOf(CommentsRsListEvent.OpenCommentDetail::class.java) - assertThat((event as CommentsRsListEvent.OpenCommentDetail).remoteCommentId).isEqualTo(42L) - assertThat(event.site).isEqualTo(site) - cancelAndIgnoreRemainingEvents() - } - } - - @Test - fun `post titles are resolved in a batch and applied to rows`() = test { - givenPage(listOf(rsItem(id = 1, postId = 10), rsItem(id = 2, postId = 20))) - whenever(commentsRsDataSource.fetchPostTitles(site, listOf(10L, 20L))) - .thenReturn(mapOf(10L to "First post", 20L to "Second post")) - val viewModel = createViewModel() - - viewModel.initTab(CommentsRsListTab.ALL) - advanceUntilIdle() - - val state = viewModel.tabStates.value.getValue(CommentsRsListTab.ALL) - assertThat(state.comments.map { it.postTitle }).containsExactly("First post", "Second post") - } - - private fun rsItem(id: Long, postId: Long = 99L) = RsCommentListItem( - remoteCommentId = id, - authorName = "Jane", - authorAvatarUrl = "https://example.com/avatar.png", - dateGmt = Date(0), - contentHtml = "

hello

", - postId = postId, - status = APPROVED - ) - - companion object { - private val FIRST_PAGE = CommentListParams() - private val NEXT_PAGE = CommentListParams(page = 2u) - private val THIRD_PAGE = CommentListParams(page = 3u) - } -} From 0c828e9ff8572c7194c75964f4e474ebe16b79da Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Fri, 3 Jul 2026 09:35:35 -0400 Subject: [PATCH 10/20] Simplify review-pass leftovers - Delete the now-unused ActivityLauncher.viewUnifiedCommentsDetails - Track title-resolve jobs as a plain per-tab Job: cancel-and-relaunch is cheap because the data source caches resolved titles - Drop the dead tab parameter from onCommentClick and the unused snackbarMessages default argument Co-Authored-By: Claude Fable 5 --- .../android/ui/ActivityLauncher.java | 6 ----- .../ui/commentsrs/CommentsRsListActivity.kt | 2 +- .../ui/commentsrs/CommentsRsListViewModel.kt | 22 +++++++------------ .../screens/CommentsRsListScreen.kt | 7 +++--- 4 files changed, 12 insertions(+), 25 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/ActivityLauncher.java b/WordPress/src/main/java/org/wordpress/android/ui/ActivityLauncher.java index 8cb33d9d805b..26f8801c91cf 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/ActivityLauncher.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/ActivityLauncher.java @@ -46,7 +46,6 @@ import org.wordpress.android.ui.blaze.blazepromote.BlazePromoteParentActivity; import org.wordpress.android.ui.bloggingprompts.promptslist.BloggingPromptsListActivity; import org.wordpress.android.ui.comments.unified.UnifiedCommentsActivity; -import org.wordpress.android.ui.comments.unified.UnifiedCommentsDetailsActivity; import org.wordpress.android.ui.commentsrs.CommentsRsListActivity; import org.wordpress.android.ui.debug.cookies.DebugCookiesActivity; import org.wordpress.android.ui.debug.preferences.DebugSharedPreferenceFlagsActivity; @@ -769,11 +768,6 @@ private static boolean shouldUseRsCommentsList(@NonNull Context context, @NonNul return entryPoint.experimentalFeatures().isEnabled(Feature.RS_UNIFIED_COMMENTS); } - public static void viewUnifiedCommentsDetails(Context context, SiteModel site, long remoteCommentId) { - Intent intent = UnifiedCommentsDetailsActivity.createIntent(context, site, remoteCommentId); - context.startActivity(intent); - } - public static void viewCurrentBlogThemes(Context context, SiteModel site) { Intent intent = new Intent(context, ThemeBrowserActivity.class); intent.putExtra(WordPress.SITE, site); diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt index 971634de3e91..c4218f09e1ac 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt @@ -50,7 +50,7 @@ class CommentsRsListActivity : BaseAppCompatActivity() { onRefreshTab = { tab -> viewModel.refreshTab(tab, isUserRefresh = true) }, onLoadMore = viewModel::loadMore, onNavigateBack = { onBackPressedDispatcher.onBackPressed() }, - onCommentClick = { commentId, _ -> viewModel.onCommentClick(commentId) } + onCommentClick = viewModel::onCommentClick ) } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt index 387262fcd218..5a52702b0a06 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt @@ -70,9 +70,8 @@ class CommentsRsListViewModel @Inject constructor( private var lastTrackedTab: CommentsRsListTab? = null - // The in-flight title-resolve job per tab, with the ids it's resolving so an identical - // request isn't cancelled and re-fired when an applied page adds nothing new. - private val resolveTitleJobs = mutableMapOf>>() + // The in-flight title-resolve job per tab. + private val resolveTitleJobs = mutableMapOf() private val _site: SiteModel? = selectedSiteRepository.getSelectedSite() private val site: SiteModel @@ -112,9 +111,8 @@ class CommentsRsListViewModel @Inject constructor( // that may have gone stale (e.g. a post renamed since it was cached). commentsRsDataSource.clearPostTitles(site) // A resolve job that started before the clear may apply pre-clear titles; cancel it - // so the post-refresh pass fetches fresh results instead of deferring to it via the - // identical-ids guard. - resolveTitleJobs[tab]?.first?.cancel() + // so the post-refresh pass fetches fresh results. + resolveTitleJobs[tab]?.cancel() } updateTabUiState(tab) { copy(isRefreshing = isUserRefresh, error = null) } fetchFirstPage(tab, showErrorSnackbar = isUserRefresh) @@ -273,20 +271,16 @@ class CommentsRsListViewModel @Inject constructor( .distinct() if (unresolvedIds.isEmpty()) return - // An applied page that added no rows (deduped away) leaves the same ids unresolved; - // let the in-flight request finish rather than cancelling and re-firing it. - val inFlight = resolveTitleJobs[tab] - if (inFlight != null && inFlight.first.isActive && inFlight.second == unresolvedIds) return - - inFlight?.first?.cancel() - val job = viewModelScope.launch { + // Cancel-and-relaunch is cheap even when the ids haven't changed: the data source + // caches resolved titles, so a re-fired request skips them. + resolveTitleJobs[tab]?.cancel() + resolveTitleJobs[tab] = viewModelScope.launch { val titles = withContext(bgDispatcher) { commentsRsDataSource.fetchPostTitles(site, unresolvedIds) } if (titles.isEmpty()) return@launch updateTabUiState(tab) { copy(comments = comments.map { it.copy(postTitle = titles[it.postId] ?: it.postTitle) }) } } - resolveTitleJobs[tab] = job to unresolvedIds } /** The server message when it sent one, otherwise a friendly offline/generic message. */ diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt index 037f0273b05f..4e53ea7f3f96 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt @@ -28,7 +28,6 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.res.stringResource import androidx.compose.ui.unit.dp import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.emptyFlow import kotlinx.coroutines.launch import org.wordpress.android.R import org.wordpress.android.ui.commentsrs.CommentsRsListTab @@ -39,13 +38,13 @@ import org.wordpress.android.ui.postsrs.SnackbarMessage @Composable fun CommentsRsListScreen( tabStates: Map, - snackbarMessages: Flow = emptyFlow(), + snackbarMessages: Flow, onInitTab: (CommentsRsListTab) -> Unit, onTabChanged: (CommentsRsListTab) -> Unit, onRefreshTab: (CommentsRsListTab) -> Unit, onLoadMore: (CommentsRsListTab) -> Unit, onNavigateBack: () -> Unit, - onCommentClick: (Long, CommentsRsListTab) -> Unit + onCommentClick: (Long) -> Unit ) { val tabs = CommentsRsListTab.entries val pagerState = rememberPagerState(pageCount = { tabs.size }) @@ -116,7 +115,7 @@ fun CommentsRsListScreen( emptyMessageResId = tab.emptyMessageResId, onRefresh = { onRefreshTab(tab) }, onLoadMore = { onLoadMore(tab) }, - onCommentClick = { commentId -> onCommentClick(commentId, tab) } + onCommentClick = onCommentClick ) } } From 8a944bc9011f68f874704ad61628d1cbf01720a7 Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Thu, 2 Jul 2026 12:26:46 -0400 Subject: [PATCH 11/20] Add batch moderation to the rs comments list (Phase 2b) Long-press a comment to enter selection mode, then tap rows to build the selection. The top bar swaps to a contextual action bar offering the same per-tab actions as the legacy list's action mode (approve/unapprove/spam/ not-spam/trash/restore/delete), with confirmation dialogs for trash and delete-permanently and COMMENT_BATCH_* analytics. Moderation runs the rs writes in parallel, aggregates any failures into a single snackbar, then refreshes all initialized tabs, since moderated comments move between them. Selection clears on tab change and after each batch action. Co-Authored-By: Claude Fable 5 --- .../ui/commentsrs/CommentsRsListActivity.kt | 13 +- .../ui/commentsrs/CommentsRsListUiState.kt | 60 +++++++ .../ui/commentsrs/CommentsRsListViewModel.kt | 135 +++++++++++++++- .../commentsrs/screens/CommentsRsListItem.kt | 53 +++++-- .../screens/CommentsRsListScreen.kt | 147 ++++++++++++++++-- .../screens/CommentsRsTabListScreen.kt | 12 +- WordPress/src/main/res/values/strings.xml | 1 + 7 files changed, 384 insertions(+), 37 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt index c4218f09e1ac..c0383c77bd42 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt @@ -41,16 +41,27 @@ class CommentsRsListActivity : BaseAppCompatActivity() { setContent { val tabStates by viewModel.tabStates.collectAsState() + val selectedIds by viewModel.selectedIds.collectAsState() + val confirmation by viewModel.pendingConfirmation.collectAsState() AppThemeM3 { CommentsRsListScreen( tabStates = tabStates, + selectedIds = selectedIds, + confirmationDialog = ConfirmationDialogState( + pending = confirmation, + onDismiss = viewModel::onDismissPendingAction + ), snackbarMessages = viewModel.snackbarMessages, onInitTab = viewModel::initTab, onTabChanged = viewModel::onTabChanged, onRefreshTab = { tab -> viewModel.refreshTab(tab, isUserRefresh = true) }, onLoadMore = viewModel::loadMore, onNavigateBack = { onBackPressedDispatcher.onBackPressed() }, - onCommentClick = viewModel::onCommentClick + onCommentClick = viewModel::onCommentClick, + onCommentLongClick = viewModel::onCommentLongClick, + onClearSelection = viewModel::onClearSelection, + onBatchAction = viewModel::onBatchAction, + onConfirmPendingAction = viewModel::onConfirmPendingAction ) } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt index 758c456cfd56..4c9afe4ab86f 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt @@ -1,5 +1,7 @@ package org.wordpress.android.ui.commentsrs +import androidx.annotation.StringRes +import org.wordpress.android.R import org.wordpress.android.fluxc.model.CommentStatus /** One comment row. */ @@ -25,3 +27,61 @@ data class CommentsTabUiState( val canLoadMore: Boolean = false, val error: String? = null ) + +/** Destructive batch actions awaiting user confirmation. */ +sealed interface PendingConfirmation { + data class Trash(val commentIds: List) : PendingConfirmation + data class Delete(val commentIds: List) : PendingConfirmation +} + +data class ConfirmationDialogState( + val pending: PendingConfirmation? = null, + val onConfirm: () -> Unit = {}, + val onDismiss: () -> Unit = {} +) + +/** + * Batch moderation actions offered in selection mode. Each maps to a target [CommentStatus] + * applied to every selected comment (except [DELETE], which deletes permanently). + */ +enum class CommentsRsBatchAction( + @StringRes val labelResId: Int, + val targetStatus: CommentStatus, + val isDestructive: Boolean = false +) { + APPROVE(R.string.mnu_comment_approve, CommentStatus.APPROVED), + UNAPPROVE(R.string.mnu_comment_unapprove, CommentStatus.UNAPPROVED), + SPAM(R.string.mnu_comment_spam, CommentStatus.SPAM), + NOT_SPAM(R.string.mnu_comment_unspam, CommentStatus.APPROVED), + TRASH(R.string.mnu_comment_trash, CommentStatus.TRASH, isDestructive = true), + UNTRASH(R.string.mnu_comment_untrash, CommentStatus.APPROVED), + DELETE(R.string.mnu_comment_delete_permanently, CommentStatus.DELETED, isDestructive = true); +} + +/** The batch actions offered for a tab's selection, mirroring the legacy action mode. */ +internal fun CommentsRsListTab.batchActions(): List = when (this) { + CommentsRsListTab.ALL -> listOf( + CommentsRsBatchAction.APPROVE, + CommentsRsBatchAction.UNAPPROVE, + CommentsRsBatchAction.SPAM, + CommentsRsBatchAction.TRASH + ) + CommentsRsListTab.PENDING -> listOf( + CommentsRsBatchAction.APPROVE, + CommentsRsBatchAction.SPAM, + CommentsRsBatchAction.TRASH + ) + CommentsRsListTab.APPROVED -> listOf( + CommentsRsBatchAction.UNAPPROVE, + CommentsRsBatchAction.SPAM, + CommentsRsBatchAction.TRASH + ) + CommentsRsListTab.SPAM -> listOf( + CommentsRsBatchAction.NOT_SPAM, + CommentsRsBatchAction.TRASH + ) + CommentsRsListTab.TRASHED -> listOf( + CommentsRsBatchAction.UNTRASH, + CommentsRsBatchAction.DELETE + ) +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt index 5a52702b0a06..307d2da39e6e 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt @@ -6,6 +6,8 @@ import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Job +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -15,11 +17,13 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import org.wordpress.android.R import org.wordpress.android.analytics.AnalyticsTracker.Stat +import org.wordpress.android.fluxc.model.CommentStatus import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.modules.BG_THREAD import org.wordpress.android.ui.comments.unified.CommentsRsDataSource import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsComment import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsCommentsPageResult +import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsResult import org.wordpress.android.ui.mysite.SelectedSiteRepository import org.wordpress.android.ui.postsrs.PostRsErrorUtils import org.wordpress.android.ui.postsrs.SnackbarMessage @@ -58,6 +62,13 @@ class CommentsRsListViewModel @Inject constructor( private val _snackbarMessages = Channel(Channel.BUFFERED) val snackbarMessages = _snackbarMessages.receiveAsFlow() + // Selection mode is active while this is non-empty. + private val _selectedIds = MutableStateFlow>(emptySet()) + val selectedIds: StateFlow> = _selectedIds.asStateFlow() + + private val _pendingConfirmation = MutableStateFlow(null) + val pendingConfirmation: StateFlow = _pendingConfirmation.asStateFlow() + // Pagination cursors: the next-page params returned with each fetched page, per tab. private val nextPageParams = mutableMapOf() @@ -96,8 +107,8 @@ class CommentsRsListViewModel @Inject constructor( /** * Re-fetches the first page for [tab]. The pull-to-refresh indicator is only shown for a - * user-initiated refresh; silent refreshes (returning from the detail) keep the current - * list on screen until the new page arrives. + * user-initiated refresh; silent refreshes (after moderation, returning from the detail) + * keep the current list on screen until the new page arrives. */ @MainThread fun refreshTab(tab: CommentsRsListTab, isUserRefresh: Boolean = false) { @@ -176,18 +187,130 @@ class CommentsRsListViewModel @Inject constructor( } } - /** Opens the rs comment detail for the tapped row. */ + /** Opens the rs comment detail for the tapped row, or toggles it while selecting. */ @MainThread fun onCommentClick(remoteCommentId: Long) { - _events.trySend(CommentsRsListEvent.OpenCommentDetail(site, remoteCommentId)) + if (_selectedIds.value.isNotEmpty()) { + toggleSelection(remoteCommentId) + } else { + _events.trySend(CommentsRsListEvent.OpenCommentDetail(site, remoteCommentId)) + } + } + + /** Enters selection mode with the long-pressed row selected. */ + @MainThread + fun onCommentLongClick(remoteCommentId: Long) { + toggleSelection(remoteCommentId) + } + + @MainThread + fun onClearSelection() { + _selectedIds.value = emptySet() + } + + /** + * Runs [action] on the selected comments, first asking for confirmation for the destructive + * ones (trash, delete) like the legacy list. + */ + @MainThread + fun onBatchAction(action: CommentsRsBatchAction, tab: CommentsRsListTab) { + val ids = _selectedIds.value.toList() + if (ids.isEmpty()) return + when (action) { + CommentsRsBatchAction.TRASH -> _pendingConfirmation.value = PendingConfirmation.Trash(ids) + CommentsRsBatchAction.DELETE -> _pendingConfirmation.value = PendingConfirmation.Delete(ids) + else -> performBatchModeration(ids, action.targetStatus, tab) + } + } + + @MainThread + fun onConfirmPendingAction(tab: CommentsRsListTab) { + when (val confirmation = _pendingConfirmation.value) { + is PendingConfirmation.Trash -> + performBatchModeration(confirmation.commentIds, CommentStatus.TRASH, tab) + is PendingConfirmation.Delete -> + performBatchModeration(confirmation.commentIds, CommentStatus.DELETED, tab) + null -> Unit + } + _pendingConfirmation.value = null + } + + @MainThread + fun onDismissPendingAction() { + _pendingConfirmation.value = null + } + + private fun toggleSelection(remoteCommentId: Long) { + _selectedIds.value = if (remoteCommentId in _selectedIds.value) { + _selectedIds.value - remoteCommentId + } else { + _selectedIds.value + remoteCommentId + } + } + + /** + * Applies [newStatus] to every comment in [ids] in parallel, then refreshes all initialized + * tabs (moderated comments move between tabs). Failures are aggregated into one snackbar. + */ + private fun performBatchModeration(ids: List, newStatus: CommentStatus, tab: CommentsRsListTab) { + if (!checkNetwork()) return + trackBatchModeration(newStatus) + onClearSelection() + updateTabUiState(tab) { copy(isRefreshing = true) } + viewModelScope.launch { + val results = withContext(bgDispatcher) { + ids.map { commentId -> + async { + if (newStatus == CommentStatus.DELETED) { + commentsRsDataSource.delete(site, commentId) + } else { + commentsRsDataSource.updateStatus(site, commentId, newStatus) + } + } + }.awaitAll() + } + val failures = results.count { it is RsResult.Error } + if (failures > 0) { + _snackbarMessages.trySend( + SnackbarMessage( + resourceProvider.getString(R.string.comments_rs_moderation_failed, failures, ids.size) + ) + ) + } + refreshAllTabs() + } + } + + private fun trackBatchModeration(newStatus: CommentStatus) { + val stat = when (newStatus) { + CommentStatus.APPROVED -> Stat.COMMENT_BATCH_APPROVED + CommentStatus.UNAPPROVED -> Stat.COMMENT_BATCH_UNAPPROVED + CommentStatus.SPAM -> Stat.COMMENT_BATCH_SPAMMED + CommentStatus.TRASH -> Stat.COMMENT_BATCH_TRASHED + CommentStatus.DELETED -> Stat.COMMENT_BATCH_DELETED + else -> return + } + analyticsTracker.track(stat) + } + + private fun checkNetwork(): Boolean { + if (!networkUtilsWrapper.isNetworkAvailable()) { + _snackbarMessages.trySend( + SnackbarMessage(resourceProvider.getString(R.string.no_network_message)) + ) + return false + } + return true } /** - * Tracks the tab the pager settled on — the same COMMENT_FILTER_CHANGED event (including the - * initial selection) the legacy list emits, so filter metrics stay comparable. + * The pager settled on a new tab: clears any active selection (like the legacy action mode) + * and tracks the same COMMENT_FILTER_CHANGED event (including the initial selection) the + * legacy list emits, so filter metrics stay comparable. */ @MainThread fun onTabChanged(tab: CommentsRsListTab) { + onClearSelection() if (tab == lastTrackedTab) return lastTrackedTab = tab analyticsTracker.track( diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt index 860c8ebaf760..5e7e79ee255f 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt @@ -1,7 +1,8 @@ package org.wordpress.android.ui.commentsrs.screens +import androidx.compose.foundation.ExperimentalFoundationApi import androidx.compose.foundation.background -import androidx.compose.foundation.clickable +import androidx.compose.foundation.combinedClickable import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.IntrinsicSize @@ -14,8 +15,10 @@ import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.width import androidx.compose.foundation.shape.CircleShape import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.filled.CheckCircle import androidx.compose.material.icons.filled.Person import androidx.compose.material3.HorizontalDivider +import androidx.compose.material3.Icon import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.runtime.Composable @@ -38,17 +41,26 @@ import org.wordpress.android.ui.commentsrs.CommentRsUiModel private val AVATAR_SIZE = 40.dp private val PENDING_INDICATOR_WIDTH = 4.dp +@OptIn(ExperimentalFoundationApi::class) @Composable fun CommentsRsListItem( comment: CommentRsUiModel, + isSelected: Boolean, onClick: () -> Unit, + onLongClick: () -> Unit, modifier: Modifier = Modifier ) { Column( modifier = modifier .fillMaxWidth() - .background(MaterialTheme.colorScheme.surface) - .clickable(onClick = onClick) + .background( + if (isSelected) { + MaterialTheme.colorScheme.surfaceVariant + } else { + MaterialTheme.colorScheme.surface + } + ) + .combinedClickable(onClick = onClick, onLongClick = onLongClick) ) { Row(modifier = Modifier.height(IntrinsicSize.Min)) { // The pending indicator: a colored bar on the row's leading edge, like the legacy @@ -66,7 +78,7 @@ fun CommentsRsListItem( ) ) Row(modifier = Modifier.padding(start = 12.dp, top = 16.dp, end = 16.dp, bottom = 16.dp)) { - CommentAvatar(comment = comment) + CommentAvatar(comment = comment, isSelected = isSelected) Column( modifier = Modifier .padding(start = 16.dp) @@ -102,18 +114,27 @@ fun CommentsRsListItem( } @Composable -private fun CommentAvatar(comment: CommentRsUiModel) { - val fallback = rememberVectorPainter(Icons.Filled.Person) - AsyncImage( - model = comment.avatarUrl.ifBlank { null }, - contentDescription = null, - contentScale = ContentScale.Crop, - fallback = fallback, - error = fallback, - modifier = Modifier - .size(AVATAR_SIZE) - .clip(CircleShape) - ) +private fun CommentAvatar(comment: CommentRsUiModel, isSelected: Boolean) { + if (isSelected) { + Icon( + imageVector = Icons.Filled.CheckCircle, + contentDescription = stringResource(R.string.comment_checkmark_desc), + tint = MaterialTheme.colorScheme.primary, + modifier = Modifier.size(AVATAR_SIZE) + ) + } else { + val fallback = rememberVectorPainter(Icons.Filled.Person) + AsyncImage( + model = comment.avatarUrl.ifBlank { null }, + contentDescription = null, + contentScale = ContentScale.Crop, + fallback = fallback, + error = fallback, + modifier = Modifier + .size(AVATAR_SIZE) + .clip(CircleShape) + ) + } } /** diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt index 4e53ea7f3f96..338e7ac6579a 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt @@ -1,5 +1,6 @@ package org.wordpress.android.ui.commentsrs.screens +import androidx.annotation.StringRes import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.padding @@ -7,6 +8,8 @@ import androidx.compose.foundation.pager.HorizontalPager import androidx.compose.foundation.pager.rememberPagerState import androidx.compose.material.icons.Icons import androidx.compose.material.icons.automirrored.filled.ArrowBack +import androidx.compose.material.icons.filled.Close +import androidx.compose.material3.AlertDialog import androidx.compose.material3.ExperimentalMaterial3Api import androidx.compose.material3.Icon import androidx.compose.material3.IconButton @@ -18,6 +21,7 @@ import androidx.compose.material3.SnackbarHostState import androidx.compose.material3.SnackbarResult import androidx.compose.material3.Tab import androidx.compose.material3.Text +import androidx.compose.material3.TextButton import androidx.compose.material3.TopAppBar import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect @@ -25,31 +29,45 @@ import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.snapshotFlow import androidx.compose.ui.Modifier +import androidx.compose.ui.graphics.Color import androidx.compose.ui.res.stringResource import androidx.compose.ui.unit.dp import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.launch import org.wordpress.android.R +import org.wordpress.android.ui.commentsrs.CommentsRsBatchAction import org.wordpress.android.ui.commentsrs.CommentsRsListTab import org.wordpress.android.ui.commentsrs.CommentsTabUiState +import org.wordpress.android.ui.commentsrs.ConfirmationDialogState +import org.wordpress.android.ui.commentsrs.PendingConfirmation +import org.wordpress.android.ui.commentsrs.batchActions import org.wordpress.android.ui.postsrs.SnackbarMessage +@Suppress("LongMethod") @OptIn(ExperimentalMaterial3Api::class) @Composable fun CommentsRsListScreen( tabStates: Map, + selectedIds: Set, + confirmationDialog: ConfirmationDialogState, snackbarMessages: Flow, onInitTab: (CommentsRsListTab) -> Unit, onTabChanged: (CommentsRsListTab) -> Unit, onRefreshTab: (CommentsRsListTab) -> Unit, onLoadMore: (CommentsRsListTab) -> Unit, onNavigateBack: () -> Unit, - onCommentClick: (Long) -> Unit + onCommentClick: (Long) -> Unit, + onCommentLongClick: (Long) -> Unit, + onClearSelection: () -> Unit, + onBatchAction: (CommentsRsBatchAction, CommentsRsListTab) -> Unit, + onConfirmPendingAction: (CommentsRsListTab) -> Unit ) { val tabs = CommentsRsListTab.entries val pagerState = rememberPagerState(pageCount = { tabs.size }) val coroutineScope = rememberCoroutineScope() + val activeTab = tabs[pagerState.settledPage] val snackbarHostState = remember { SnackbarHostState() } + val isSelectionActive = selectedIds.isNotEmpty() LaunchedEffect(snackbarMessages) { snackbarMessages.collect { msg -> @@ -66,17 +84,26 @@ fun CommentsRsListScreen( Scaffold( snackbarHost = { SnackbarHost(snackbarHostState) }, topBar = { - TopAppBar( - title = { Text(text = stringResource(R.string.comments)) }, - navigationIcon = { - IconButton(onClick = onNavigateBack) { - Icon( - Icons.AutoMirrored.Filled.ArrowBack, - contentDescription = stringResource(R.string.back) - ) + if (isSelectionActive) { + SelectionTopBar( + selectedCount = selectedIds.size, + actions = activeTab.batchActions(), + onClearSelection = onClearSelection, + onBatchAction = { action -> onBatchAction(action, activeTab) } + ) + } else { + TopAppBar( + title = { Text(text = stringResource(R.string.comments)) }, + navigationIcon = { + IconButton(onClick = onNavigateBack) { + Icon( + Icons.AutoMirrored.Filled.ArrowBack, + contentDescription = stringResource(R.string.back) + ) + } } - } - ) + ) + } } ) { contentPadding -> Column(modifier = Modifier.fillMaxSize().padding(contentPadding)) { @@ -113,11 +140,107 @@ fun CommentsRsListScreen( CommentsRsTabListScreen( state = tabState, emptyMessageResId = tab.emptyMessageResId, + selectedIds = selectedIds, onRefresh = { onRefreshTab(tab) }, onLoadMore = { onLoadMore(tab) }, - onCommentClick = onCommentClick + onCommentClick = onCommentClick, + onCommentLongClick = onCommentLongClick ) } } } + + when (val pending = confirmationDialog.pending) { + is PendingConfirmation.Trash -> ConfirmationDialog( + titleResId = R.string.trash, + message = stringResource(R.string.dlg_confirm_trash_comments), + confirmTextResId = R.string.dlg_confirm_action_trash, + isDestructive = true, + onConfirm = { onConfirmPendingAction(activeTab) }, + onDismiss = confirmationDialog.onDismiss + ) + is PendingConfirmation.Delete -> ConfirmationDialog( + titleResId = R.string.delete, + message = stringResource( + if (pending.commentIds.size > 1) { + R.string.dlg_sure_to_delete_comments + } else { + R.string.dlg_sure_to_delete_comment + } + ), + confirmTextResId = R.string.delete, + isDestructive = true, + onConfirm = { onConfirmPendingAction(activeTab) }, + onDismiss = confirmationDialog.onDismiss + ) + null -> {} + } +} + +@OptIn(ExperimentalMaterial3Api::class) +@Composable +private fun SelectionTopBar( + selectedCount: Int, + actions: List, + onClearSelection: () -> Unit, + onBatchAction: (CommentsRsBatchAction) -> Unit +) { + TopAppBar( + title = { Text(text = stringResource(R.string.cab_selected, selectedCount)) }, + navigationIcon = { + IconButton(onClick = onClearSelection) { + Icon( + Icons.Default.Close, + contentDescription = stringResource(R.string.clear) + ) + } + }, + actions = { + actions.forEach { action -> + TextButton(onClick = { onBatchAction(action) }) { + Text( + text = stringResource(action.labelResId), + color = if (action.isDestructive) { + MaterialTheme.colorScheme.error + } else { + MaterialTheme.colorScheme.primary + } + ) + } + } + } + ) +} + +@Composable +private fun ConfirmationDialog( + @StringRes titleResId: Int, + message: String, + @StringRes confirmTextResId: Int, + isDestructive: Boolean = false, + onConfirm: () -> Unit, + onDismiss: () -> Unit +) { + AlertDialog( + onDismissRequest = onDismiss, + title = { Text(stringResource(titleResId)) }, + text = { Text(message) }, + confirmButton = { + TextButton(onClick = onConfirm) { + Text( + stringResource(confirmTextResId), + color = if (isDestructive) { + MaterialTheme.colorScheme.error + } else { + Color.Unspecified + } + ) + } + }, + dismissButton = { + TextButton(onClick = onDismiss) { + Text(stringResource(R.string.cancel)) + } + } + ) } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt index ea83ebaf4806..da87afc71425 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt @@ -44,9 +44,11 @@ import org.wordpress.android.ui.compose.components.ShimmerBox fun CommentsRsTabListScreen( state: CommentsTabUiState, emptyMessageResId: Int, + selectedIds: Set, onRefresh: () -> Unit, onLoadMore: () -> Unit, onCommentClick: (Long) -> Unit, + onCommentLongClick: (Long) -> Unit, modifier: Modifier = Modifier ) { val pullToRefreshState = rememberPullToRefreshState() @@ -76,10 +78,12 @@ fun CommentsRsTabListScreen( ) else -> CommentListContent( comments = state.comments, + selectedIds = selectedIds, isLoadingMore = state.isLoadingMore, canLoadMore = state.canLoadMore, onLoadMore = onLoadMore, - onCommentClick = onCommentClick + onCommentClick = onCommentClick, + onCommentLongClick = onCommentLongClick ) } } @@ -88,10 +92,12 @@ fun CommentsRsTabListScreen( @Composable private fun CommentListContent( comments: List, + selectedIds: Set, isLoadingMore: Boolean, canLoadMore: Boolean, onLoadMore: () -> Unit, - onCommentClick: (Long) -> Unit + onCommentClick: (Long) -> Unit, + onCommentLongClick: (Long) -> Unit ) { val listState = rememberLazyListState() @@ -121,7 +127,9 @@ private fun CommentListContent( ) { comment -> CommentsRsListItem( comment = comment, + isSelected = comment.remoteCommentId in selectedIds, onClick = { onCommentClick(comment.remoteCommentId) }, + onLongClick = { onCommentLongClick(comment.remoteCommentId) }, modifier = Modifier.animateItem() ) } diff --git a/WordPress/src/main/res/values/strings.xml b/WordPress/src/main/res/values/strings.xml index c200d82bd0a2..e9e1ea696a59 100644 --- a/WordPress/src/main/res/values/strings.xml +++ b/WordPress/src/main/res/values/strings.xml @@ -434,6 +434,7 @@ No unreplied comments No spam comments No trashed comments + %1$d of %2$d comments couldn\'t be updated Reply to %s Comment trashed Comment marked as spam From 8d90ca5a61d2f65b37bf4987f037bb99c6ff67e4 Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Thu, 2 Jul 2026 12:28:16 -0400 Subject: [PATCH 12/20] Add search to the rs comments list (Phase 2c) Search icon in the top bar opens an inline query field (like the rs posts list): queries are debounced 250ms with a 3-character minimum, keep the active tab's status filter, and clear back to the normal tab content when search closes. Selection mode is cleared when search opens or closes. Co-Authored-By: Claude Fable 5 --- .../ui/commentsrs/CommentsRsListActivity.kt | 7 + .../ui/commentsrs/CommentsRsListViewModel.kt | 73 ++++++++- .../screens/CommentsRsListScreen.kt | 142 +++++++++++++++--- .../screens/CommentsRsTabListScreen.kt | 11 +- WordPress/src/main/res/values/strings.xml | 2 + 5 files changed, 207 insertions(+), 28 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt index c0383c77bd42..7a479c760e4c 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt @@ -41,17 +41,24 @@ class CommentsRsListActivity : BaseAppCompatActivity() { setContent { val tabStates by viewModel.tabStates.collectAsState() + val isSearchActive by viewModel.isSearchActive.collectAsState() + val searchQuery by viewModel.searchQuery.collectAsState() val selectedIds by viewModel.selectedIds.collectAsState() val confirmation by viewModel.pendingConfirmation.collectAsState() AppThemeM3 { CommentsRsListScreen( tabStates = tabStates, + isSearchActive = isSearchActive, + searchQuery = searchQuery, selectedIds = selectedIds, confirmationDialog = ConfirmationDialogState( pending = confirmation, onDismiss = viewModel::onDismissPendingAction ), snackbarMessages = viewModel.snackbarMessages, + onSearchOpen = viewModel::onSearchOpen, + onSearchQueryChanged = viewModel::onSearchQueryChanged, + onSearchClose = viewModel::onSearchClose, onInitTab = viewModel::initTab, onTabChanged = viewModel::onTabChanged, onRefreshTab = { tab -> viewModel.refreshTab(tab, isUserRefresh = true) }, diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt index 307d2da39e6e..76ed84d1e6ea 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt @@ -5,6 +5,7 @@ import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.Job import kotlinx.coroutines.async import kotlinx.coroutines.awaitAll @@ -12,6 +13,8 @@ import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.debounce +import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.receiveAsFlow import kotlinx.coroutines.launch import kotlinx.coroutines.withContext @@ -56,6 +59,13 @@ class CommentsRsListViewModel @Inject constructor( private val _tabStates = MutableStateFlow>(emptyMap()) val tabStates: StateFlow> = _tabStates.asStateFlow() + private val _isSearchActive = MutableStateFlow(false) + val isSearchActive: StateFlow = _isSearchActive.asStateFlow() + + private val _searchQuery = MutableStateFlow("") + val searchQuery: StateFlow = _searchQuery.asStateFlow() + private var activeSearchTab = CommentsRsListTab.ALL + private val _events = Channel(Channel.BUFFERED) val events = _events.receiveAsFlow() @@ -92,6 +102,17 @@ class CommentsRsListViewModel @Inject constructor( if (_site == null) { _events.trySend(CommentsRsListEvent.ShowToast(R.string.blog_not_found)) _events.trySend(CommentsRsListEvent.Finish) + } else { + @OptIn(FlowPreview::class) + viewModelScope.launch { + _searchQuery + .debounce(SEARCH_DEBOUNCE_MS) + .filter { it.length >= MIN_SEARCH_QUERY_LENGTH } + .collect { + clearTabs() + initTab(activeSearchTab) + } + } } } @@ -319,9 +340,44 @@ class CommentsRsListViewModel @Inject constructor( ) } + /** Clears all tab states so the list appears empty while the user types a query. */ + @MainThread + fun onSearchOpen() { + onClearSelection() + _isSearchActive.value = true + clearTabs() + } + + /** + * Updates the search query. Non-blank queries are debounced before triggering an API call. + * Blank queries immediately clear results so the idle state appears without delay. + */ + @MainThread + fun onSearchQueryChanged(query: String, activeTab: CommentsRsListTab) { + activeSearchTab = activeTab + _searchQuery.value = query + if (query.isBlank()) clearTabs() + } + + /** + * Closes search mode: clears the query and immediately re-initializes [activeTab] so the + * normal tab content appears without debounce delay. + */ + @MainThread + fun onSearchClose(activeTab: CommentsRsListTab) { + onClearSelection() + _isSearchActive.value = false + _searchQuery.value = "" + clearTabs() + initTab(activeTab) + } + private fun fetchFirstPage(tab: CommentsRsListTab, showErrorSnackbar: Boolean = true) { firstPageJobs[tab] = viewModelScope.launch { - val params = commentsRsDataSource.firstPageParams(status = tab.queryStatus) + val params = commentsRsDataSource.firstPageParams( + status = tab.queryStatus, + search = _searchQuery.value.ifBlank { null } + ) val result = withContext(bgDispatcher) { commentsRsDataSource.fetchCommentsPage(site, params) } when (result) { is RsCommentsPageResult.Success -> applyPage(tab, result, append = false) @@ -406,6 +462,18 @@ class CommentsRsListViewModel @Inject constructor( } } + private fun clearTabs() { + nextPageParams.clear() + pageGenerations.clear() + // Cancel in-flight fetches so a page requested before the clear can't repopulate the + // emptied tabs with pre-search (or pre-close) content. + firstPageJobs.values.forEach { it.cancel() } + firstPageJobs.clear() + resolveTitleJobs.values.forEach { it.cancel() } + resolveTitleJobs.clear() + _tabStates.value = emptyMap() + } + /** The server message when it sent one, otherwise a friendly offline/generic message. */ private fun errorMessage(serverMessage: String?): String = serverMessage?.takeIf { it.isNotBlank() } @@ -435,5 +503,8 @@ class CommentsRsListViewModel @Inject constructor( // Same property key as the legacy list's tracking (UnifiedCommentsActivity). private const val SELECTED_FILTER_PROPERTY = "selected_filter" + + private const val SEARCH_DEBOUNCE_MS = 250L + internal const val MIN_SEARCH_QUERY_LENGTH = 3 } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt index 338e7ac6579a..89d3c4505a91 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt @@ -3,12 +3,16 @@ package org.wordpress.android.ui.commentsrs.screens import androidx.annotation.StringRes import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.padding import androidx.compose.foundation.pager.HorizontalPager import androidx.compose.foundation.pager.rememberPagerState +import androidx.compose.foundation.text.KeyboardActions +import androidx.compose.foundation.text.KeyboardOptions import androidx.compose.material.icons.Icons import androidx.compose.material.icons.automirrored.filled.ArrowBack import androidx.compose.material.icons.filled.Close +import androidx.compose.material.icons.filled.Search import androidx.compose.material3.AlertDialog import androidx.compose.material3.ExperimentalMaterial3Api import androidx.compose.material3.Icon @@ -22,6 +26,8 @@ import androidx.compose.material3.SnackbarResult import androidx.compose.material3.Tab import androidx.compose.material3.Text import androidx.compose.material3.TextButton +import androidx.compose.material3.TextField +import androidx.compose.material3.TextFieldDefaults import androidx.compose.material3.TopAppBar import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect @@ -29,28 +35,38 @@ import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.snapshotFlow import androidx.compose.ui.Modifier +import androidx.compose.ui.focus.FocusRequester +import androidx.compose.ui.focus.focusRequester import androidx.compose.ui.graphics.Color +import androidx.compose.ui.platform.LocalFocusManager import androidx.compose.ui.res.stringResource +import androidx.compose.ui.text.input.ImeAction import androidx.compose.ui.unit.dp import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.launch import org.wordpress.android.R import org.wordpress.android.ui.commentsrs.CommentsRsBatchAction import org.wordpress.android.ui.commentsrs.CommentsRsListTab +import org.wordpress.android.ui.commentsrs.CommentsRsListViewModel.Companion.MIN_SEARCH_QUERY_LENGTH import org.wordpress.android.ui.commentsrs.CommentsTabUiState import org.wordpress.android.ui.commentsrs.ConfirmationDialogState import org.wordpress.android.ui.commentsrs.PendingConfirmation import org.wordpress.android.ui.commentsrs.batchActions import org.wordpress.android.ui.postsrs.SnackbarMessage -@Suppress("LongMethod") +@Suppress("CyclomaticComplexMethod", "LongMethod") @OptIn(ExperimentalMaterial3Api::class) @Composable fun CommentsRsListScreen( tabStates: Map, + isSearchActive: Boolean, + searchQuery: String, selectedIds: Set, confirmationDialog: ConfirmationDialogState, snackbarMessages: Flow, + onSearchOpen: () -> Unit, + onSearchQueryChanged: (String, CommentsRsListTab) -> Unit, + onSearchClose: (CommentsRsListTab) -> Unit, onInitTab: (CommentsRsListTab) -> Unit, onTabChanged: (CommentsRsListTab) -> Unit, onRefreshTab: (CommentsRsListTab) -> Unit, @@ -65,6 +81,7 @@ fun CommentsRsListScreen( val tabs = CommentsRsListTab.entries val pagerState = rememberPagerState(pageCount = { tabs.size }) val coroutineScope = rememberCoroutineScope() + val focusRequester = remember { FocusRequester() } val activeTab = tabs[pagerState.settledPage] val snackbarHostState = remember { SnackbarHostState() } val isSelectionActive = selectedIds.isNotEmpty() @@ -92,34 +109,35 @@ fun CommentsRsListScreen( onBatchAction = { action -> onBatchAction(action, activeTab) } ) } else { - TopAppBar( - title = { Text(text = stringResource(R.string.comments)) }, - navigationIcon = { - IconButton(onClick = onNavigateBack) { - Icon( - Icons.AutoMirrored.Filled.ArrowBack, - contentDescription = stringResource(R.string.back) - ) - } - } + SearchableTopBar( + isSearchActive = isSearchActive, + searchQuery = searchQuery, + activeTab = activeTab, + focusRequester = focusRequester, + onSearchOpen = onSearchOpen, + onSearchQueryChanged = onSearchQueryChanged, + onSearchClose = onSearchClose, + onNavigateBack = onNavigateBack ) } } ) { contentPadding -> Column(modifier = Modifier.fillMaxSize().padding(contentPadding)) { - PrimaryScrollableTabRow( - selectedTabIndex = pagerState.settledPage, - edgePadding = 0.dp - ) { - tabs.forEachIndexed { index, tab -> - Tab( - selected = pagerState.settledPage == index, - onClick = { - coroutineScope.launch { pagerState.animateScrollToPage(index) } - }, - unselectedContentColor = MaterialTheme.colorScheme.onSurfaceVariant, - text = { Text(text = stringResource(tab.labelResId)) } - ) + if (!isSearchActive) { + PrimaryScrollableTabRow( + selectedTabIndex = pagerState.settledPage, + edgePadding = 0.dp + ) { + tabs.forEachIndexed { index, tab -> + Tab( + selected = pagerState.settledPage == index, + onClick = { + coroutineScope.launch { pagerState.animateScrollToPage(index) } + }, + unselectedContentColor = MaterialTheme.colorScheme.onSurfaceVariant, + text = { Text(text = stringResource(tab.labelResId)) } + ) + } } } @@ -132,7 +150,8 @@ fun CommentsRsListScreen( HorizontalPager( state = pagerState, - modifier = Modifier.weight(1f) + modifier = Modifier.weight(1f), + userScrollEnabled = !isSearchActive ) { page -> val tab = tabs[page] val tabState = tabStates[tab] ?: CommentsTabUiState(isLoading = true) @@ -141,6 +160,8 @@ fun CommentsRsListScreen( state = tabState, emptyMessageResId = tab.emptyMessageResId, selectedIds = selectedIds, + isSearchIdle = isSearchActive && searchQuery.length < MIN_SEARCH_QUERY_LENGTH, + isSearching = isSearchActive && searchQuery.length >= MIN_SEARCH_QUERY_LENGTH, onRefresh = { onRefreshTab(tab) }, onLoadMore = { onLoadMore(tab) }, onCommentClick = onCommentClick, @@ -177,6 +198,77 @@ fun CommentsRsListScreen( } } +@OptIn(ExperimentalMaterial3Api::class) +@Composable +private fun SearchableTopBar( + isSearchActive: Boolean, + searchQuery: String, + activeTab: CommentsRsListTab, + focusRequester: FocusRequester, + onSearchOpen: () -> Unit, + onSearchQueryChanged: (String, CommentsRsListTab) -> Unit, + onSearchClose: (CommentsRsListTab) -> Unit, + onNavigateBack: () -> Unit +) { + val focusManager = LocalFocusManager.current + TopAppBar( + title = { + if (isSearchActive) { + TextField( + value = searchQuery, + onValueChange = { query -> onSearchQueryChanged(query, activeTab) }, + placeholder = { Text(stringResource(R.string.comments_rs_search_prompt)) }, + singleLine = true, + keyboardOptions = KeyboardOptions.Default.copy(imeAction = ImeAction.Search), + keyboardActions = KeyboardActions(onSearch = { focusManager.clearFocus() }), + colors = TextFieldDefaults.colors( + focusedContainerColor = Color.Transparent, + unfocusedContainerColor = Color.Transparent, + focusedIndicatorColor = Color.Transparent, + unfocusedIndicatorColor = Color.Transparent + ), + modifier = Modifier.fillMaxWidth().focusRequester(focusRequester) + ) + } else { + Text(text = stringResource(R.string.comments)) + } + }, + navigationIcon = { + IconButton(onClick = { + if (isSearchActive) onSearchClose(activeTab) else onNavigateBack() + }) { + Icon( + Icons.AutoMirrored.Filled.ArrowBack, + contentDescription = stringResource(R.string.back) + ) + } + }, + actions = { + if (isSearchActive) { + if (searchQuery.isNotEmpty()) { + IconButton(onClick = { onSearchQueryChanged("", activeTab) }) { + Icon( + Icons.Default.Close, + contentDescription = stringResource(R.string.clear) + ) + } + } + } else { + IconButton(onClick = onSearchOpen) { + Icon( + Icons.Default.Search, + contentDescription = stringResource(R.string.comments_rs_search_prompt) + ) + } + } + } + ) + + if (isSearchActive) { + LaunchedEffect(Unit) { focusRequester.requestFocus() } + } +} + @OptIn(ExperimentalMaterial3Api::class) @Composable private fun SelectionTopBar( diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt index da87afc71425..7e0b905c5fab 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt @@ -49,7 +49,9 @@ fun CommentsRsTabListScreen( onLoadMore: () -> Unit, onCommentClick: (Long) -> Unit, onCommentLongClick: (Long) -> Unit, - modifier: Modifier = Modifier + modifier: Modifier = Modifier, + isSearchIdle: Boolean = false, + isSearching: Boolean = false ) { val pullToRefreshState = rememberPullToRefreshState() @@ -68,13 +70,18 @@ fun CommentsRsTabListScreen( } ) { when { + isSearchIdle -> Box(Modifier.fillMaxSize()) state.isLoading -> ShimmerList() state.error != null && state.comments.isEmpty() -> ErrorContent( error = state.error, onRetry = onRefresh ) state.comments.isEmpty() && !state.isRefreshing -> EmptyContent( - emptyMessageResId = emptyMessageResId + emptyMessageResId = if (isSearching) { + R.string.comments_rs_search_nothing_found + } else { + emptyMessageResId + } ) else -> CommentListContent( comments = state.comments, diff --git a/WordPress/src/main/res/values/strings.xml b/WordPress/src/main/res/values/strings.xml index e9e1ea696a59..b380977ef0a0 100644 --- a/WordPress/src/main/res/values/strings.xml +++ b/WordPress/src/main/res/values/strings.xml @@ -434,6 +434,8 @@ No unreplied comments No spam comments No trashed comments + Search comments + No comments matching your search %1$d of %2$d comments couldn\'t be updated Reply to %s Comment trashed From 44a56860b98073ee92de3edfdb753a07fcfbb5e3 Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Tue, 7 Jul 2026 09:31:45 -0400 Subject: [PATCH 13/20] Fix search review findings: clear races, scoping, scroll and focus MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Track and cancel in-flight load-more jobs in clearTabs(): a survivor could land on a reset page-generation counter and append its stale (unfiltered) page into fresh search results, corrupting the cursor chain - Don't resurrect a cleared tab from the stale-load-more discard branch; the ghost entry blocked initTab and left the tab shimmering forever - Keep the tab row visible while searching: the comments endpoint accepts a single status per request (unlike posts), so a search is always scoped to one tab — visible tabs make that scope explicit and switchable - Reset each tab's scroll position when search opens, changes query, or closes, so replaced content renders at the top instead of clamping to the old offset and tripping the load-more trigger - Use trimmed query lengths for the debounce filter and the idle/searching thresholds, so whitespace-only queries can't fetch the unfiltered list - Clear results on any below-minimum query (not just blank) so the previous query's rows can't flash as matches for the next one - Guard initTab against fetching during search-idle (Activity recreation or a late pager settle would bypass the debounce and minimum length) - Only clear the selection on a real tab change, not the settled-page re-emission after recreation, and request search focus once per open so a selection round-trip doesn't pop the keyboard Co-Authored-By: Claude Fable 5 --- .../ui/commentsrs/CommentsRsListViewModel.kt | 42 +++++++--- .../screens/CommentsRsListScreen.kt | 82 ++++++++++++------- 2 files changed, 86 insertions(+), 38 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt index 13ec7659031b..18998d08b7a4 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt @@ -90,6 +90,11 @@ class CommentsRsListViewModel @Inject constructor( // The in-flight first-page fetch per tab, so overlapping refreshes don't duplicate it. private val firstPageJobs = mutableMapOf() + // The in-flight load-more fetch per tab, so clearTabs() can cancel it: a load-more that + // survived a clear could otherwise land on a reset page-generation counter and append its + // stale rows into the freshly fetched list. + private val loadMoreJobs = mutableMapOf() + private var lastTrackedTab: CommentsRsListTab? = null // The in-flight title-resolve job per tab. @@ -108,7 +113,10 @@ class CommentsRsListViewModel @Inject constructor( viewModelScope.launch { _searchQuery .debounce(SEARCH_DEBOUNCE_MS) - .filter { it.length >= MIN_SEARCH_QUERY_LENGTH } + // Trimmed: fetchFirstPage maps a blank query to "no search", so a + // whitespace-only query passing a raw length check would fetch (and present) + // the full unfiltered list as search results. + .filter { it.trim().length >= MIN_SEARCH_QUERY_LENGTH } .collect { clearTabs() initTab(activeSearchTab) @@ -126,14 +134,16 @@ class CommentsRsListViewModel @Inject constructor( } /** - * Updates the search query. Non-blank queries are debounced before triggering an API call. - * Blank queries immediately clear results so the idle state appears without delay. + * Updates the search query. Searchable queries are debounced before triggering an API call. + * A query below the searchable minimum clears immediately: the previous query's results + * must not flash as matches for the next query in the debounce window after the user types + * back up to the minimum. */ @MainThread fun onSearchQueryChanged(query: String, activeTab: CommentsRsListTab) { activeSearchTab = activeTab _searchQuery.value = query - if (query.isBlank()) clearTabs() + if (query.trim().length < MIN_SEARCH_QUERY_LENGTH && _tabStates.value.isNotEmpty()) clearTabs() } /** @@ -151,13 +161,15 @@ class CommentsRsListViewModel @Inject constructor( /** * Drops every tab's rows and pagination bookkeeping so tabs re-initialize from scratch - * (entering, changing, or leaving a search). Cancelling the in-flight jobs keeps a fetch - * started before the clear from resurrecting a stale tab afterwards; a load-more in flight - * is discarded by the page-generation check once its generation entry is gone. + * (entering, changing, or leaving a search). Every in-flight fetch is cancelled: a fetch + * started before the clear must not resurrect a stale tab, and a surviving load-more would + * land on a reset generation counter and mix its stale page into the fresh list. */ private fun clearTabs() { firstPageJobs.values.forEach { it.cancel() } firstPageJobs.clear() + loadMoreJobs.values.forEach { it.cancel() } + loadMoreJobs.clear() resolveTitleJobs.values.forEach { it.cancel() } resolveTitleJobs.clear() nextPageParams.clear() @@ -171,6 +183,10 @@ class CommentsRsListViewModel @Inject constructor( // updateTabUiState inserts the tab synchronously, so this also blocks re-entry while // the first fetch is in flight. if (_tabStates.value.containsKey(tab)) return + // While a search is open with a below-minimum query, the screen shows the idle state and + // the debounce collector hasn't fired — an init here (pager settle, Activity recreation) + // would fetch unfiltered or below-minimum results behind the idle screen. + if (_isSearchActive.value && _searchQuery.value.trim().length < MIN_SEARCH_QUERY_LENGTH) return updateTabUiState(tab) { copy(isLoading = true) } fetchFirstPage(tab) } @@ -224,12 +240,16 @@ class CommentsRsListViewModel @Inject constructor( updateTabUiState(tab) { copy(isLoadingMore = true) } val generation = pageGenerations[tab] - viewModelScope.launch { + loadMoreJobs[tab] = viewModelScope.launch { val result = withContext(bgDispatcher) { commentsRsDataSource.fetchCommentsPage(site, params) } if (pageGenerations[tab] != generation) { // A refresh replaced the list while this page was in flight, so its cursor is // stale; appending it would mix old and new pages and corrupt the cursor chain. - updateTabUiState(tab) { copy(isLoadingMore = false) } + // Only touch tabs that still exist: writing through updateTabUiState after a + // clearTabs() would resurrect the tab as a ghost entry that blocks initTab. + if (_tabStates.value.containsKey(tab)) { + updateTabUiState(tab) { copy(isLoadingMore = false) } + } return@launch } when (result) { @@ -384,8 +404,10 @@ class CommentsRsListViewModel @Inject constructor( */ @MainThread fun onTabChanged(tab: CommentsRsListTab) { - onClearSelection() + // A re-emission of the same tab (Activity recreation restarting the pager's settled-page + // flow) is not a tab change: it must neither re-track nor clear an active selection. if (tab == lastTrackedTab) return + onClearSelection() lastTrackedTab = tab analyticsTracker.track( Stat.COMMENT_FILTER_CHANGED, diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt index 4357bfeeb6b3..0882b6709e6b 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt @@ -123,11 +123,22 @@ fun CommentsRsListScreen( .toSet() val isSelectionActive = selectedStatuses.isNotEmpty() val focusRequester = remember { FocusRequester() } + // Focus (and the keyboard) is requested once per search open, not every time SearchTopBar + // re-enters composition — a selection round-trip over active search must not pop the keyboard. + var searchFocusPending by remember { mutableStateOf(false) } val topBarMode = when { isSelectionActive -> TopBarMode.SELECTION isSearchActive -> TopBarMode.SEARCH else -> TopBarMode.NORMAL } + val trimmedQueryLength = searchQuery.trim().length + + // Search opening, closing, or changing the query replaces each tab's content wholesale; reset + // the scroll so page 1 renders at the top. The hoisted list states otherwise keep their old + // deep offsets, clamping the short new list to its end and tripping the load-more trigger. + LaunchedEffect(isSearchActive, searchQuery) { + listStates.values.forEach { it.scrollToItem(0) } + } // Like the legacy action mode: the first back press dismisses the selection, the next one // leaves the screen. Enabled on selectedIds (not selectedStatuses) so any live selection is @@ -167,6 +178,8 @@ fun CommentsRsListScreen( TopBarMode.SEARCH -> SearchTopBar( searchQuery = searchQuery, focusRequester = focusRequester, + requestFocus = searchFocusPending, + onFocusHandled = { searchFocusPending = false }, onQueryChanged = { query -> onSearchQueryChanged(query, activeTab) }, onClose = { onSearchClose(activeTab) } ) @@ -181,7 +194,10 @@ fun CommentsRsListScreen( } }, actions = { - IconButton(onClick = onSearchOpen) { + IconButton(onClick = { + searchFocusPending = true + onSearchOpen() + }) { Icon( Icons.Default.Search, contentDescription = stringResource(R.string.comments_rs_search_prompt) @@ -194,28 +210,30 @@ fun CommentsRsListScreen( } ) { contentPadding -> Column(modifier = Modifier.fillMaxSize().padding(contentPadding)) { - if (!isSearchActive) { - PrimaryScrollableTabRow( - selectedTabIndex = pagerState.settledPage, - edgePadding = 0.dp - ) { - tabs.forEachIndexed { index, tab -> - Tab( - selected = pagerState.settledPage == index, - onClick = { - coroutineScope.launch { - if (pagerState.settledPage == index) { - // Re-tapping the active tab scrolls its list back to the top. - listStates.getValue(tab).animateScrollToItem(0) - } else { - pagerState.animateScrollToPage(index) - } + // The tab row stays visible while searching: the comments endpoint only accepts a + // single status per request (unlike posts, which search across all statuses), so a + // search is always scoped to one tab — keeping the tabs on screen makes that scope + // visible and lets the user re-run the query against another status. + PrimaryScrollableTabRow( + selectedTabIndex = pagerState.settledPage, + edgePadding = 0.dp + ) { + tabs.forEachIndexed { index, tab -> + Tab( + selected = pagerState.settledPage == index, + onClick = { + coroutineScope.launch { + if (pagerState.settledPage == index) { + // Re-tapping the active tab scrolls its list back to the top. + listStates.getValue(tab).animateScrollToItem(0) + } else { + pagerState.animateScrollToPage(index) } - }, - unselectedContentColor = MaterialTheme.colorScheme.onSurfaceVariant, - text = { Text(text = stringResource(tab.labelResId)) } - ) - } + } + }, + unselectedContentColor = MaterialTheme.colorScheme.onSurfaceVariant, + text = { Text(text = stringResource(tab.labelResId)) } + ) } } @@ -228,8 +246,7 @@ fun CommentsRsListScreen( HorizontalPager( state = pagerState, - modifier = Modifier.weight(1f), - userScrollEnabled = !isSearchActive + modifier = Modifier.weight(1f) ) { page -> val tab = tabs[page] val tabState = tabStates[tab] ?: CommentsTabUiState(isLoading = true) @@ -239,8 +256,8 @@ fun CommentsRsListScreen( emptyMessageResId = tab.emptyMessageResId, selectedIds = selectedIds, listState = listStates.getValue(tab), - isSearchIdle = isSearchActive && searchQuery.length < MIN_SEARCH_QUERY_LENGTH, - isSearching = isSearchActive && searchQuery.length >= MIN_SEARCH_QUERY_LENGTH, + isSearchIdle = isSearchActive && trimmedQueryLength < MIN_SEARCH_QUERY_LENGTH, + isSearching = isSearchActive && trimmedQueryLength >= MIN_SEARCH_QUERY_LENGTH, onRefresh = { onRefreshTab(tab) }, onLoadMore = { onLoadMore(tab) }, onCommentClick = onCommentClick, @@ -329,13 +346,17 @@ private fun SelectionTopBar( /** * Search mode: an inline query field replaces the title, the navigation icon closes the search, - * and a clear button empties a non-blank query (like the rs posts list search). + * and a clear button empties a non-blank query (like the rs posts list search). Focus is only + * requested when [requestFocus] is set (once per search open) so re-entering composition — e.g. + * returning from the selection top bar — doesn't pop the keyboard uninvited. */ @OptIn(ExperimentalMaterial3Api::class) @Composable private fun SearchTopBar( searchQuery: String, focusRequester: FocusRequester, + requestFocus: Boolean, + onFocusHandled: () -> Unit, onQueryChanged: (String) -> Unit, onClose: () -> Unit ) { @@ -378,7 +399,12 @@ private fun SearchTopBar( } ) - LaunchedEffect(Unit) { focusRequester.requestFocus() } + LaunchedEffect(requestFocus) { + if (requestFocus) { + focusRequester.requestFocus() + onFocusHandled() + } + } } @Composable From 68ae9900f7e2ff934caa5187dcab1a347501803a Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Tue, 7 Jul 2026 12:34:53 -0400 Subject: [PATCH 14/20] Address second-pass review findings on comments list search - Re-init the tab the user is looking at (lastTrackedTab, which follows every pager settle) instead of the tab captured at keystroke time; a tab switch inside the debounce window previously left the visible tab as a permanent shimmer with no fetch in flight. Removes the activeSearchTab field and the tab parameter from onSearchQueryChanged - Trim and dedupe the query pipeline so whitespace-only edits neither restart the debounce nor refetch, and send the trimmed query to the API - Clear displayed rows on any trimmed-query change, covering whole-string replacements (select-all + paste, keyboard suggestions, voice input) that never dip below the minimum length - Rework the scroll reset: skip the initial emission so Activity recreation keeps the restored scroll positions, and skip lists already at the top to avoid pointless forced remeasures and fling cancellation - Don't re-select failed batch-moderation ids into a search session opened while the batch ran; the untracked job outlives clearTabs and an invisible selection would swallow back presses and hijack row taps - Consolidate the minimum-length policy into one isSearchable predicate exposed as isQuerySearchable, so the screen no longer re-derives it from the ViewModel's companion constant Co-Authored-By: Claude Fable 5 --- .../ui/commentsrs/CommentsRsListActivity.kt | 2 + .../ui/commentsrs/CommentsRsListViewModel.kt | 60 +++++++++++++------ .../screens/CommentsRsListScreen.kt | 36 +++++++---- 3 files changed, 70 insertions(+), 28 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt index 091f06320b95..43be22243395 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt @@ -45,6 +45,7 @@ class CommentsRsListActivity : BaseAppCompatActivity() { val confirmation by viewModel.pendingConfirmation.collectAsState() val isSearchActive by viewModel.isSearchActive.collectAsState() val searchQuery by viewModel.searchQuery.collectAsState() + val isQuerySearchable by viewModel.isQuerySearchable.collectAsState() AppThemeM3 { CommentsRsListScreen( tabStates = tabStates, @@ -52,6 +53,7 @@ class CommentsRsListActivity : BaseAppCompatActivity() { pendingConfirmation = confirmation, isSearchActive = isSearchActive, searchQuery = searchQuery, + isQuerySearchable = isQuerySearchable, onDismissConfirmation = viewModel::onDismissPendingAction, snackbarMessages = viewModel.snackbarMessages, onSearchOpen = viewModel::onSearchOpen, diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt index 18998d08b7a4..871f212471a9 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt @@ -11,11 +11,15 @@ import kotlinx.coroutines.async import kotlinx.coroutines.awaitAll import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.debounce +import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.filter +import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.receiveAsFlow +import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import kotlinx.coroutines.withContext @@ -65,7 +69,15 @@ class CommentsRsListViewModel @Inject constructor( private val _searchQuery = MutableStateFlow("") val searchQuery: StateFlow = _searchQuery.asStateFlow() - private var activeSearchTab = CommentsRsListTab.ALL + + /** + * Whether the current query is long enough to search. The single owner of the minimum-length + * policy — the debounce filter, the [initTab] guard, and the screen's idle state all derive + * from it rather than re-implementing the trim-and-compare rule. + */ + val isQuerySearchable: StateFlow = _searchQuery + .map { isSearchable(it) } + .stateIn(viewModelScope, SharingStarted.Eagerly, false) private val _events = Channel(Channel.BUFFERED) val events = _events.receiveAsFlow() @@ -112,14 +124,20 @@ class CommentsRsListViewModel @Inject constructor( @OptIn(FlowPreview::class) viewModelScope.launch { _searchQuery + // Trim + dedupe first so whitespace-only edits ("abc" -> "abc ") neither + // restart the debounce nor refetch a semantically identical query; trimming + // also keeps a whitespace-only query from passing the length filter and + // fetching (and presenting) the full unfiltered list as search results. + .map { it.trim() } + .distinctUntilChanged() .debounce(SEARCH_DEBOUNCE_MS) - // Trimmed: fetchFirstPage maps a blank query to "no search", so a - // whitespace-only query passing a raw length check would fetch (and present) - // the full unfiltered list as search results. - .filter { it.trim().length >= MIN_SEARCH_QUERY_LENGTH } + .filter { isSearchable(it) } .collect { clearTabs() - initTab(activeSearchTab) + // lastTrackedTab follows the pager (updated on every settle), so this + // targets the tab the user is actually looking at even when they switch + // tabs inside the debounce window. + initTab(lastTrackedTab ?: CommentsRsListTab.ALL) } } } @@ -135,15 +153,16 @@ class CommentsRsListViewModel @Inject constructor( /** * Updates the search query. Searchable queries are debounced before triggering an API call. - * A query below the searchable minimum clears immediately: the previous query's results - * must not flash as matches for the next query in the debounce window after the user types - * back up to the minimum. + * Any change to the trimmed query clears the current rows immediately: displayed comments + * always belong to the last executed search, and must not linger as apparent matches for a + * different query — whether the user deleted below the minimum or replaced the whole query + * at once (select-all + paste, a keyboard suggestion, voice input). */ @MainThread - fun onSearchQueryChanged(query: String, activeTab: CommentsRsListTab) { - activeSearchTab = activeTab + fun onSearchQueryChanged(query: String) { + val previous = _searchQuery.value _searchQuery.value = query - if (query.trim().length < MIN_SEARCH_QUERY_LENGTH && _tabStates.value.isNotEmpty()) clearTabs() + if (query.trim() != previous.trim()) clearTabs() } /** @@ -186,7 +205,7 @@ class CommentsRsListViewModel @Inject constructor( // While a search is open with a below-minimum query, the screen shows the idle state and // the debounce collector hasn't fired — an init here (pager settle, Activity recreation) // would fetch unfiltered or below-minimum results behind the idle screen. - if (_isSearchActive.value && _searchQuery.value.trim().length < MIN_SEARCH_QUERY_LENGTH) return + if (_isSearchActive.value && !isSearchable(_searchQuery.value)) return updateTabUiState(tab) { copy(isLoading = true) } fetchFirstPage(tab) } @@ -368,8 +387,13 @@ class CommentsRsListViewModel @Inject constructor( } _snackbarMessages.trySend(SnackbarMessage(message)) // Failed comments kept their status and place in the list, so re-selecting them - // lets the user retry immediately instead of hunting them down again. - _selectedIds.value = failedIds.toSet() + // lets the user retry immediately instead of hunting them down again — unless a + // search was opened while the batch ran: this job outlives clearTabs(), and + // re-selecting rows that are no longer displayed would leave an invisible + // selection intercepting back presses and row taps. + if (!_isSearchActive.value) { + _selectedIds.value = failedIds.toSet() + } } refreshAllTabs() } @@ -419,7 +443,7 @@ class CommentsRsListViewModel @Inject constructor( firstPageJobs[tab] = viewModelScope.launch { val params = commentsRsDataSource.firstPageParams( status = tab.queryStatus, - search = _searchQuery.value.ifBlank { null } + search = _searchQuery.value.trim().ifBlank { null } ) val result = withContext(bgDispatcher) { commentsRsDataSource.fetchCommentsPage(site, params) } when (result) { @@ -520,6 +544,8 @@ class CommentsRsListViewModel @Inject constructor( postId = postId ) + private fun isSearchable(query: String) = query.trim().length >= MIN_SEARCH_QUERY_LENGTH + private fun getTabUiState(tab: CommentsRsListTab): CommentsTabUiState = _tabStates.value[tab] ?: CommentsTabUiState(isLoading = true) @@ -536,6 +562,6 @@ class CommentsRsListViewModel @Inject constructor( private const val SELECTED_FILTER_PROPERTY = "selected_filter" private const val SEARCH_DEBOUNCE_MS = 250L - internal const val MIN_SEARCH_QUERY_LENGTH = 3 + private const val MIN_SEARCH_QUERY_LENGTH = 3 } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt index 0882b6709e6b..6ec3cf7aea9e 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt @@ -42,6 +42,7 @@ import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope +import androidx.compose.runtime.rememberUpdatedState import androidx.compose.runtime.setValue import androidx.compose.runtime.snapshotFlow import androidx.compose.ui.Modifier @@ -54,12 +55,12 @@ import androidx.compose.ui.res.stringResource import androidx.compose.ui.text.input.ImeAction import androidx.compose.ui.unit.dp import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.drop import kotlinx.coroutines.launch import org.wordpress.android.R import org.wordpress.android.fluxc.model.CommentStatus import org.wordpress.android.ui.commentsrs.CommentsRsBatchAction import org.wordpress.android.ui.commentsrs.CommentsRsListTab -import org.wordpress.android.ui.commentsrs.CommentsRsListViewModel.Companion.MIN_SEARCH_QUERY_LENGTH import org.wordpress.android.ui.commentsrs.CommentsTabUiState import org.wordpress.android.ui.commentsrs.PendingConfirmation import org.wordpress.android.ui.commentsrs.batchActions @@ -82,10 +83,11 @@ fun CommentsRsListScreen( pendingConfirmation: PendingConfirmation?, isSearchActive: Boolean, searchQuery: String, + isQuerySearchable: Boolean, onDismissConfirmation: () -> Unit, snackbarMessages: Flow, onSearchOpen: () -> Unit, - onSearchQueryChanged: (String, CommentsRsListTab) -> Unit, + onSearchQueryChanged: (String) -> Unit, onSearchClose: (CommentsRsListTab) -> Unit, onInitTab: (CommentsRsListTab) -> Unit, onTabChanged: (CommentsRsListTab) -> Unit, @@ -131,13 +133,25 @@ fun CommentsRsListScreen( isSearchActive -> TopBarMode.SEARCH else -> TopBarMode.NORMAL } - val trimmedQueryLength = searchQuery.trim().length - // Search opening, closing, or changing the query replaces each tab's content wholesale; reset - // the scroll so page 1 renders at the top. The hoisted list states otherwise keep their old - // deep offsets, clamping the short new list to its end and tripping the load-more trigger. - LaunchedEffect(isSearchActive, searchQuery) { - listStates.values.forEach { it.scrollToItem(0) } + // Search opening, closing, or changing the trimmed query replaces each tab's content + // wholesale; reset the scroll so page 1 renders at the top. The hoisted list states otherwise + // keep their old deep offsets, clamping the short new list to its end and tripping the + // load-more trigger. drop(1) skips the initial emission: an Activity recreation must not + // discard the scroll positions the saveable list states just restored. The at-top check + // avoids scrollToItem's forced remeasure (and fling cancellation) when there's nothing to do. + val currentSearchActive by rememberUpdatedState(isSearchActive) + val currentQuery by rememberUpdatedState(searchQuery) + LaunchedEffect(Unit) { + snapshotFlow { currentSearchActive to currentQuery.trim() } + .drop(1) + .collect { + listStates.values.forEach { state -> + if (state.firstVisibleItemIndex > 0 || state.firstVisibleItemScrollOffset > 0) { + state.scrollToItem(0) + } + } + } } // Like the legacy action mode: the first back press dismisses the selection, the next one @@ -180,7 +194,7 @@ fun CommentsRsListScreen( focusRequester = focusRequester, requestFocus = searchFocusPending, onFocusHandled = { searchFocusPending = false }, - onQueryChanged = { query -> onSearchQueryChanged(query, activeTab) }, + onQueryChanged = onSearchQueryChanged, onClose = { onSearchClose(activeTab) } ) TopBarMode.NORMAL -> TopAppBar( @@ -256,8 +270,8 @@ fun CommentsRsListScreen( emptyMessageResId = tab.emptyMessageResId, selectedIds = selectedIds, listState = listStates.getValue(tab), - isSearchIdle = isSearchActive && trimmedQueryLength < MIN_SEARCH_QUERY_LENGTH, - isSearching = isSearchActive && trimmedQueryLength >= MIN_SEARCH_QUERY_LENGTH, + isSearchIdle = isSearchActive && !isQuerySearchable, + isSearching = isSearchActive && isQuerySearchable, onRefresh = { onRefreshTab(tab) }, onLoadMore = { onLoadMore(tab) }, onCommentClick = onCommentClick, From 5296d0d576f7dec3ace8798863d45fae595d3a60 Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Tue, 7 Jul 2026 12:54:44 -0400 Subject: [PATCH 15/20] Address third-pass review findings on comments list search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Guard the failed-batch re-selection with a clear-generation counter instead of the instantaneous search flag: a batch run inside an unchanged search keeps the retry re-selection (its failed rows are still displayed), while any clearTabs() during the batch — search opened, closed, or query changed — suppresses it, closing the invisible-selection hole when search was opened and closed mid-batch - Unify the search-query policy in one flow: a shared trim+dedupe normalization feeds an immediate on-change clear and the debounced fetch, so the two halves can't drift; onSearchQueryChanged shrinks to a plain setter. The collector no longer re-clears, so a pager settle that already fetched the same query inside the debounce window keeps its results instead of being cancelled and refetched - Track the pager's current tab in a dedicated field instead of reusing the analytics dedupe key, so tracking changes can't silently retarget the debounced search - Render the blank search-idle screen (not the animated shimmer) for tabs cleared while awaiting the debounce, removing the per-keystroke results/shimmer flicker; shimmer now means an actual fetch in flight - Pass isSearchActive/isQuerySearchable to the tab screen and derive the idle/searching split inside it, removing the representable-but- impossible flag combination - Hoist the once-per-open focus request out of SearchTopBar, dropping its two ferry parameters Co-Authored-By: Claude Fable 5 --- .../ui/commentsrs/CommentsRsListViewModel.kt | 63 ++++++++++++------- .../screens/CommentsRsListScreen.kt | 44 +++++++------ .../screens/CommentsRsTabListScreen.kt | 32 ++++++---- 3 files changed, 83 insertions(+), 56 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt index 871f212471a9..77238c2a1b2e 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt @@ -16,8 +16,10 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.drop import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.receiveAsFlow import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.update @@ -107,11 +109,20 @@ class CommentsRsListViewModel @Inject constructor( // stale rows into the freshly fetched list. private val loadMoreJobs = mutableMapOf() + // The tab the pager last settled on. Kept separate from [lastTrackedTab] (the analytics + // dedupe key) so changes to tracking behavior can't silently change which tab a debounced + // search targets. + private var currentTab: CommentsRsListTab? = null + private var lastTrackedTab: CommentsRsListTab? = null // The in-flight title-resolve job per tab. private val resolveTitleJobs = mutableMapOf() + // Bumped by clearTabs(). Long-lived jobs (batch moderation) capture it at launch and skip + // their completion-time state writes when it moved: the rows they refer to are gone. + private var clearGeneration = 0 + private val _site: SiteModel? = selectedSiteRepository.getSelectedSite() private val site: SiteModel get() = requireNotNull(_site) { "No selected site — Activity should have finished" } @@ -124,20 +135,30 @@ class CommentsRsListViewModel @Inject constructor( @OptIn(FlowPreview::class) viewModelScope.launch { _searchQuery - // Trim + dedupe first so whitespace-only edits ("abc" -> "abc ") neither - // restart the debounce nor refetch a semantically identical query; trimming - // also keeps a whitespace-only query from passing the length filter and - // fetching (and presenting) the full unfiltered list as search results. + // One shared normalization decides both halves of the search policy below: + // trim + dedupe so whitespace-only edits ("abc" -> "abc ") neither clear nor + // refetch, and so a whitespace-only query can't pass the length filter and + // fetch (and present) the full unfiltered list as search results. .map { it.trim() } .distinctUntilChanged() + // Skip the initial "" so opening the screen doesn't clear the loading tabs. + .drop(1) + // Clear immediately on any change: displayed rows always belong to the last + // executed search and must not linger as apparent matches for a different + // query — whether edited below the minimum or replaced wholesale (select-all + // + paste, a keyboard suggestion, voice input). The isSearchActive guard + // keeps the close transition intact: onSearchClose resets the query itself + // and rebuilds the tabs synchronously. + .onEach { if (_isSearchActive.value) clearTabs() } .debounce(SEARCH_DEBOUNCE_MS) .filter { isSearchable(it) } .collect { - clearTabs() - // lastTrackedTab follows the pager (updated on every settle), so this - // targets the tab the user is actually looking at even when they switch - // tabs inside the debounce window. - initTab(lastTrackedTab ?: CommentsRsListTab.ALL) + // No clear here: every mutation path cleared at mutation time, so any + // surviving tab content was already fetched with this query (e.g. by a + // pager settle inside the debounce window) and initTab's containsKey + // guard keeps it. currentTab follows the pager, so this targets the tab + // the user is actually looking at. + initTab(currentTab ?: CommentsRsListTab.ALL) } } } @@ -152,17 +173,12 @@ class CommentsRsListViewModel @Inject constructor( } /** - * Updates the search query. Searchable queries are debounced before triggering an API call. - * Any change to the trimmed query clears the current rows immediately: displayed comments - * always belong to the last executed search, and must not linger as apparent matches for a - * different query — whether the user deleted below the minimum or replaced the whole query - * at once (select-all + paste, a keyboard suggestion, voice input). + * Updates the search query. The pipeline in `init` reacts to it: an immediate clear on any + * trimmed change, then a debounced fetch once the query is searchable. */ @MainThread fun onSearchQueryChanged(query: String) { - val previous = _searchQuery.value _searchQuery.value = query - if (query.trim() != previous.trim()) clearTabs() } /** @@ -185,6 +201,7 @@ class CommentsRsListViewModel @Inject constructor( * land on a reset generation counter and mix its stale page into the fresh list. */ private fun clearTabs() { + clearGeneration++ firstPageJobs.values.forEach { it.cancel() } firstPageJobs.clear() loadMoreJobs.values.forEach { it.cancel() } @@ -363,6 +380,7 @@ class CommentsRsListViewModel @Inject constructor( trackBatchModeration(newStatus) onClearSelection() updateTabUiState(tab) { copy(isRefreshing = true) } + val generationAtStart = clearGeneration viewModelScope.launch { val results = withContext(bgDispatcher) { ids.map { commentId -> @@ -387,11 +405,13 @@ class CommentsRsListViewModel @Inject constructor( } _snackbarMessages.trySend(SnackbarMessage(message)) // Failed comments kept their status and place in the list, so re-selecting them - // lets the user retry immediately instead of hunting them down again — unless a - // search was opened while the batch ran: this job outlives clearTabs(), and - // re-selecting rows that are no longer displayed would leave an invisible - // selection intercepting back presses and row taps. - if (!_isSearchActive.value) { + // lets the user retry immediately instead of hunting them down again — unless + // clearTabs() ran while the batch was in flight (search opened, closed, or query + // changed): this job outlives the clear, and re-selecting rows that may no longer + // be displayed would leave an invisible selection intercepting back presses and + // row taps. A batch run inside an unchanged search keeps the affordance: its + // failed rows are still in the refreshed results. + if (clearGeneration == generationAtStart) { _selectedIds.value = failedIds.toSet() } } @@ -428,6 +448,7 @@ class CommentsRsListViewModel @Inject constructor( */ @MainThread fun onTabChanged(tab: CommentsRsListTab) { + currentTab = tab // A re-emission of the same tab (Activity recreation restarting the pager's settled-page // flow) is not a tab change: it must neither re-track nor clear an active selection. if (tab == lastTrackedTab) return diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt index 6ec3cf7aea9e..f1df55844347 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt @@ -189,14 +189,21 @@ fun CommentsRsListScreen( onClearSelection = onClearSelection, onBatchAction = { action -> onBatchAction(action, activeTab) } ) - TopBarMode.SEARCH -> SearchTopBar( - searchQuery = searchQuery, - focusRequester = focusRequester, - requestFocus = searchFocusPending, - onFocusHandled = { searchFocusPending = false }, - onQueryChanged = onSearchQueryChanged, - onClose = { onSearchClose(activeTab) } - ) + TopBarMode.SEARCH -> { + SearchTopBar( + searchQuery = searchQuery, + focusRequester = focusRequester, + onQueryChanged = onSearchQueryChanged, + onClose = { onSearchClose(activeTab) } + ) + // Runs after SearchTopBar is composed, so the requester is attached. + LaunchedEffect(searchFocusPending) { + if (searchFocusPending) { + focusRequester.requestFocus() + searchFocusPending = false + } + } + } TopBarMode.NORMAL -> TopAppBar( title = { Text(text = stringResource(R.string.comments)) }, navigationIcon = { @@ -263,15 +270,14 @@ fun CommentsRsListScreen( modifier = Modifier.weight(1f) ) { page -> val tab = tabs[page] - val tabState = tabStates[tab] ?: CommentsTabUiState(isLoading = true) CommentsRsTabListScreen( - state = tabState, + state = tabStates[tab], emptyMessageResId = tab.emptyMessageResId, selectedIds = selectedIds, listState = listStates.getValue(tab), - isSearchIdle = isSearchActive && !isQuerySearchable, - isSearching = isSearchActive && isQuerySearchable, + isSearchActive = isSearchActive, + isQuerySearchable = isQuerySearchable, onRefresh = { onRefreshTab(tab) }, onLoadMore = { onLoadMore(tab) }, onCommentClick = onCommentClick, @@ -360,17 +366,15 @@ private fun SelectionTopBar( /** * Search mode: an inline query field replaces the title, the navigation icon closes the search, - * and a clear button empties a non-blank query (like the rs posts list search). Focus is only - * requested when [requestFocus] is set (once per search open) so re-entering composition — e.g. - * returning from the selection top bar — doesn't pop the keyboard uninvited. + * and a clear button empties a non-blank query (like the rs posts list search). The caller owns + * the once-per-open focus request so re-entering composition — e.g. returning from the selection + * top bar — doesn't pop the keyboard uninvited. */ @OptIn(ExperimentalMaterial3Api::class) @Composable private fun SearchTopBar( searchQuery: String, focusRequester: FocusRequester, - requestFocus: Boolean, - onFocusHandled: () -> Unit, onQueryChanged: (String) -> Unit, onClose: () -> Unit ) { @@ -413,12 +417,6 @@ private fun SearchTopBar( } ) - LaunchedEffect(requestFocus) { - if (requestFocus) { - focusRequester.requestFocus() - onFocusHandled() - } - } } @Composable diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt index b6d8f4618e9a..6e310db3aa7e 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt @@ -42,7 +42,8 @@ import org.wordpress.android.ui.compose.components.ShimmerBox @OptIn(ExperimentalMaterial3Api::class) @Composable fun CommentsRsTabListScreen( - state: CommentsTabUiState, + /** Null when the tab isn't initialized: first composition, or cleared awaiting a search. */ + state: CommentsTabUiState?, emptyMessageResId: Int, selectedIds: Set, listState: LazyListState, @@ -51,20 +52,27 @@ fun CommentsRsTabListScreen( onCommentClick: (Long) -> Unit, onCommentLongClick: (Long) -> Unit, modifier: Modifier = Modifier, - isSearchIdle: Boolean = false, - isSearching: Boolean = false + isSearchActive: Boolean = false, + isQuerySearchable: Boolean = false ) { + // While searching, a missing tab state means "cleared, waiting for the debounced fetch" + // (initTab inserts an isLoading state the moment it actually fetches) — show the same blank + // idle screen as a below-minimum query rather than flashing the animated shimmer on every + // keystroke. Outside search, a missing state is the pre-init first composition: shimmer. + val isSearchIdle = isSearchActive && (!isQuerySearchable || state == null) + val isSearching = isSearchActive && isQuerySearchable + val tabState = state ?: CommentsTabUiState(isLoading = true) val pullToRefreshState = rememberPullToRefreshState() PullToRefreshBox( modifier = modifier.fillMaxSize(), - isRefreshing = state.isRefreshing, + isRefreshing = tabState.isRefreshing, state = pullToRefreshState, onRefresh = onRefresh, indicator = { PullToRefreshDefaults.Indicator( state = pullToRefreshState, - isRefreshing = state.isRefreshing, + isRefreshing = tabState.isRefreshing, color = MaterialTheme.colorScheme.primary, modifier = Modifier.align(Alignment.TopCenter) ) @@ -74,12 +82,12 @@ fun CommentsRsTabListScreen( // Search is open but the query is still below the minimum length: show nothing // rather than a misleading "no comments" state. isSearchIdle -> Box(Modifier.fillMaxSize()) - state.isLoading -> ShimmerList() - state.error != null && state.comments.isEmpty() -> ErrorContent( - error = state.error, + tabState.isLoading -> ShimmerList() + tabState.error != null && tabState.comments.isEmpty() -> ErrorContent( + error = tabState.error, onRetry = onRefresh ) - state.comments.isEmpty() && !state.isRefreshing -> EmptyContent( + tabState.comments.isEmpty() && !tabState.isRefreshing -> EmptyContent( emptyMessageResId = if (isSearching) { R.string.comments_rs_search_nothing_found } else { @@ -87,11 +95,11 @@ fun CommentsRsTabListScreen( } ) else -> CommentListContent( - comments = state.comments, + comments = tabState.comments, selectedIds = selectedIds, listState = listState, - isLoadingMore = state.isLoadingMore, - canLoadMore = state.canLoadMore, + isLoadingMore = tabState.isLoadingMore, + canLoadMore = tabState.canLoadMore, onLoadMore = onLoadMore, onCommentClick = onCommentClick, onCommentLongClick = onCommentLongClick From 1e1baf6e580c245e7364a3588933908cf558b0bb Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Tue, 7 Jul 2026 15:12:04 -0400 Subject: [PATCH 16/20] Remove stray blank line before brace (checkstyle) Left behind when SearchTopBar's focus effect was hoisted to the caller. Co-Authored-By: Claude Fable 5 --- .../android/ui/commentsrs/screens/CommentsRsListScreen.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt index f1df55844347..d4cf7ce590f9 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt @@ -416,7 +416,6 @@ private fun SearchTopBar( } } ) - } @Composable From 7afff50cb213916e41e8f697231b25ca49db35f4 Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Wed, 8 Jul 2026 06:21:47 -0400 Subject: [PATCH 17/20] Add note mode to the rs comment detail ViewModel When started with a noteId, the ViewModel refreshes the note DB after every successful write, fires commentModerated for the notifications result contract, edits via NotificationCommentIdentifier, and falls back to Note.hasLikedComment() when the FluxC cache row is missing. Co-Authored-By: Claude Fable 5 --- .../wrappers/NotificationsTableWrapper.kt | 2 + .../unified/UnifiedCommentDetailsViewModel.kt | 92 +++++++++++++--- .../UnifiedCommentDetailsViewModelTest.kt | 100 +++++++++++++++++- 3 files changed, 177 insertions(+), 17 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/datasets/wrappers/NotificationsTableWrapper.kt b/WordPress/src/main/java/org/wordpress/android/datasets/wrappers/NotificationsTableWrapper.kt index 8c667439ad2c..542f01854697 100644 --- a/WordPress/src/main/java/org/wordpress/android/datasets/wrappers/NotificationsTableWrapper.kt +++ b/WordPress/src/main/java/org/wordpress/android/datasets/wrappers/NotificationsTableWrapper.kt @@ -12,4 +12,6 @@ class NotificationsTableWrapper @Inject constructor() { fun saveNotes(notes: List, clearBeforeSaving: Boolean) { NotificationsTable.saveNotes(notes, clearBeforeSaving) } + + fun getNoteById(noteId: String): Note? = NotificationsTable.getNoteById(noteId) } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsViewModel.kt index 22f8d361d34d..a2a801e91f65 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsViewModel.kt @@ -5,6 +5,7 @@ import androidx.lifecycle.MutableLiveData import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.withContext import org.wordpress.android.R +import org.wordpress.android.datasets.wrappers.NotificationsTableWrapper import org.wordpress.android.fluxc.model.CommentStatus import org.wordpress.android.fluxc.model.CommentStatus.APPROVED import org.wordpress.android.fluxc.model.CommentStatus.DELETED @@ -21,9 +22,11 @@ import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent.Close import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent.LaunchEditComment import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent.OpenPostInReader import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent.ReplySent +import org.wordpress.android.ui.comments.unified.CommentIdentifier.NotificationCommentIdentifier import org.wordpress.android.ui.comments.unified.CommentIdentifier.SiteCommentIdentifier import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsComment import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsResult +import org.wordpress.android.ui.notifications.utils.NotificationsActionsWrapper import org.wordpress.android.ui.pages.SnackbarMessageHolder import org.wordpress.android.ui.utils.UiString import org.wordpress.android.ui.utils.UiString.UiStringRes @@ -36,7 +39,7 @@ import javax.inject.Inject import javax.inject.Named /** - * ViewModel for the unified comment detail screen (site-comments path). + * ViewModel for the unified comment detail screen. * * Loads and mutates the comment through wordpress-rs ([CommentsRsDataSource]) — view, moderation, * reply, edit. Two things stay on FluxC because wordpress-rs can't cover them: @@ -46,7 +49,11 @@ import javax.inject.Named * list reflects it. The cache row is also the source of the post title, like state and local id * used to launch the (still FluxC) edit screen. * - * Notification-sourced comments are handled in a later phase of the Comments Unification migration. + * **Note mode**: when [start] receives a `noteId` the comment was opened from a notification. + * Every successful write then also refreshes the note DB (so the notifications list stays fresh) + * and moderation additionally fires [commentModerated], which the host turns into the + * notifications-list result extras. Edits use [NotificationCommentIdentifier] so the edit screen + * refreshes the note too. */ class UnifiedCommentDetailsViewModel @Inject constructor( @Named(UI_THREAD) private val mainDispatcher: CoroutineDispatcher, @@ -55,12 +62,15 @@ class UnifiedCommentDetailsViewModel @Inject constructor( private val commentsStore: CommentsStore, private val localCommentCacheUpdateHandler: LocalCommentCacheUpdateHandler, private val networkUtilsWrapper: NetworkUtilsWrapper, - private val dateTimeUtilsWrapper: DateTimeUtilsWrapper + private val dateTimeUtilsWrapper: DateTimeUtilsWrapper, + private val notificationsActionsWrapper: NotificationsActionsWrapper, + private val notificationsTableWrapper: NotificationsTableWrapper ) : ScopedViewModel(mainDispatcher) { private val _uiState = MutableLiveData() private val _uiActionEvent = MutableLiveData>() private val _onSnackbarMessage = MutableLiveData>() private val _commentChanged = MutableLiveData>() + private val _commentModerated = MutableLiveData>() val uiState: LiveData = _uiState val uiActionEvent: LiveData> = _uiActionEvent @@ -73,20 +83,29 @@ class UnifiedCommentDetailsViewModel @Inject constructor( */ val commentChanged: LiveData> = _commentChanged + /** + * Fires with the new status when the comment was successfully moderated — note mode only. + * The notifications host reports it via the result extras the notifications list uses to + * update the moderated note's row. + */ + val commentModerated: LiveData> = _commentModerated + private var isStarted = false private lateinit var site: SiteModel private var remoteCommentId: Long = 0 + private var noteId: String? = null private var loadedComment: RsComment? = null private var isLikeInProgress = false // From the FluxC cache row: the local id used to launch the (still FluxC) edit screen. private var localCommentId: Int = 0 - fun start(site: SiteModel, remoteCommentId: Long) { + fun start(site: SiteModel, remoteCommentId: Long, noteId: String? = null) { if (isStarted) return isStarted = true this.site = site this.remoteCommentId = remoteCommentId + this.noteId = noteId loadComment() } @@ -110,7 +129,7 @@ class UnifiedCommentDetailsViewModel @Inject constructor( if (!isRefresh) { _uiState.value = CommentDetailsUiState(showProgress = true) } - val (rsComment, cached, fallbackPostTitle) = withContext(bgDispatcher) { + val loaded = withContext(bgDispatcher) { val rs = commentsRsDataSource.getComment(site, remoteCommentId) var local = commentsStore.getCommentByLocalSiteAndRemoteId(site.id, remoteCommentId).firstOrNull() // Opened from the rs list the FluxC cache may not have this comment at all (the @@ -129,13 +148,24 @@ class UnifiedCommentDetailsViewModel @Inject constructor( } else { "" } - Triple(rs, local, fallbackTitle) + // No cache row to read the like state from (the fetch failed or was skipped): in + // note mode the note itself knows whether the comment is liked. + val likedFallback = if (local == null) { + noteId?.let { notificationsTableWrapper.getNoteById(it)?.hasLikedComment() } ?: false + } else { + false + } + CommentLoadResult(rs, local, fallbackTitle, likedFallback) } when { - rsComment != null -> { - loadedComment = rsComment - localCommentId = cached?.id?.toInt() ?: 0 - _uiState.value = rsComment.toUiState(cached, fallbackPostTitle) + loaded.rsComment != null -> { + loadedComment = loaded.rsComment + localCommentId = loaded.cached?.id?.toInt() ?: 0 + _uiState.value = loaded.rsComment.toUiState( + loaded.cached, + loaded.fallbackPostTitle, + loaded.fallbackIsLiked + ) } isRefresh -> showSnackbar(R.string.error_load_comment) else -> _onSnackbarMessage.value = Event( @@ -185,7 +215,10 @@ class UnifiedCommentDetailsViewModel @Inject constructor( _uiState.value = _uiState.value?.copy(isLiked = !isLike) showSnackbar(R.string.error_generic) } else { - withContext(bgDispatcher) { localCommentCacheUpdateHandler.requestCommentsUpdate() } + withContext(bgDispatcher) { + localCommentCacheUpdateHandler.requestCommentsUpdate() + refreshNote() + } } isLikeInProgress = false } @@ -193,9 +226,11 @@ class UnifiedCommentDetailsViewModel @Inject constructor( fun onEditClicked() { if (loadedComment == null) return - _uiActionEvent.value = Event( - LaunchEditComment(site, SiteCommentIdentifier(localCommentId, remoteCommentId)) - ) + // In note mode edit through the note identifier so the edit screen refreshes the note DB + // after saving, keeping the notifications list consistent with the edited comment. + val identifier = noteId?.let { NotificationCommentIdentifier(it, remoteCommentId) } + ?: SiteCommentIdentifier(localCommentId, remoteCommentId) + _uiActionEvent.value = Event(LaunchEditComment(site, identifier)) } fun onPostTitleClicked() { @@ -247,6 +282,8 @@ class UnifiedCommentDetailsViewModel @Inject constructor( val result = withContext(bgDispatcher) { moderate(APPROVED) } if (result is RsResult.Error) { _uiState.value = _uiState.value?.copy(status = UNAPPROVED) + } else if (noteId != null) { + _commentModerated.value = Event(APPROVED) } } @@ -265,6 +302,9 @@ class UnifiedCommentDetailsViewModel @Inject constructor( showError(result.message, R.string.error_moderate_comment) } else { _commentChanged.value = Event(Unit) + if (noteId != null) { + _commentModerated.value = Event(newStatus) + } if (closeOnSuccess) { _uiActionEvent.value = Event(Close) } @@ -292,10 +332,19 @@ class UnifiedCommentDetailsViewModel @Inject constructor( commentsStore.moderateCommentLocally(site, remoteCommentId, newStatus) } localCommentCacheUpdateHandler.requestCommentsUpdate() + refreshNote() } return result } + /** + * Note mode only: re-downloads the note so the note DB (which feeds the notifications list) + * reflects the change just made to the comment. + */ + private suspend fun refreshNote() { + noteId?.let { notificationsActionsWrapper.downloadNoteAndUpdateDB(it) } + } + private fun currentStatus(): CommentStatus = _uiState.value?.status ?: CommentStatus.ALL private fun isOffline(): Boolean { @@ -314,7 +363,11 @@ class UnifiedCommentDetailsViewModel @Inject constructor( _onSnackbarMessage.value = Event(SnackbarMessageHolder(uiMessage)) } - private fun RsComment.toUiState(cached: CommentEntity?, fallbackPostTitle: String) = CommentDetailsUiState( + private fun RsComment.toUiState( + cached: CommentEntity?, + fallbackPostTitle: String, + fallbackIsLiked: Boolean + ) = CommentDetailsUiState( showProgress = false, contentVisible = true, authorName = authorName, @@ -324,7 +377,14 @@ class UnifiedCommentDetailsViewModel @Inject constructor( postTitle = cached?.postTitle?.takeIf { it.isNotBlank() } ?: fallbackPostTitle, commentUrl = url, status = status, - isLiked = cached?.iLike ?: false + isLiked = cached?.iLike ?: fallbackIsLiked + ) + + private data class CommentLoadResult( + val rsComment: RsComment?, + val cached: CommentEntity?, + val fallbackPostTitle: String, + val fallbackIsLiked: Boolean ) data class CommentDetailsUiState( diff --git a/WordPress/src/test/java/org/wordpress/android/ui/comments/viewmodels/UnifiedCommentDetailsViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/comments/viewmodels/UnifiedCommentDetailsViewModelTest.kt index 9880c54ce1d1..e2fd54afd29a 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/comments/viewmodels/UnifiedCommentDetailsViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/comments/viewmodels/UnifiedCommentDetailsViewModelTest.kt @@ -10,10 +10,14 @@ import org.mockito.kotlin.any import org.mockito.kotlin.atLeastOnce import org.mockito.kotlin.doSuspendableAnswer import org.mockito.kotlin.eq +import org.mockito.kotlin.mock import org.mockito.kotlin.times import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoInteractions import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest +import org.wordpress.android.datasets.wrappers.NotificationsTableWrapper +import org.wordpress.android.fluxc.model.CommentStatus import org.wordpress.android.fluxc.model.CommentStatus.APPROVED import org.wordpress.android.fluxc.model.CommentStatus.SPAM import org.wordpress.android.fluxc.model.CommentStatus.TRASH @@ -23,18 +27,21 @@ import org.wordpress.android.fluxc.persistence.comments.CommentsDao.CommentEntit import org.wordpress.android.fluxc.store.CommentsStore import org.wordpress.android.fluxc.store.CommentsStore.CommentsActionPayload import org.wordpress.android.fluxc.store.CommentsStore.CommentsData.CommentsActionData +import org.wordpress.android.models.Note import org.wordpress.android.models.usecases.LocalCommentCacheUpdateHandler import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent.Close import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent.LaunchEditComment import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent.OpenPostInReader import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent.ReplySent +import org.wordpress.android.ui.comments.unified.CommentIdentifier.NotificationCommentIdentifier import org.wordpress.android.ui.comments.unified.CommentIdentifier.SiteCommentIdentifier import org.wordpress.android.ui.comments.unified.CommentsRsDataSource import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsComment import org.wordpress.android.ui.comments.unified.CommentsRsDataSource.RsResult import org.wordpress.android.ui.comments.unified.UnifiedCommentDetailsViewModel import org.wordpress.android.ui.comments.unified.UnifiedCommentDetailsViewModel.CommentDetailsUiState +import org.wordpress.android.ui.notifications.utils.NotificationsActionsWrapper import org.wordpress.android.ui.pages.SnackbarMessageHolder import org.wordpress.android.ui.utils.UiString.UiStringText import org.wordpress.android.util.DateTimeUtilsWrapper @@ -58,11 +65,18 @@ class UnifiedCommentDetailsViewModelTest : BaseUnitTest() { @Mock lateinit var dateTimeUtilsWrapper: DateTimeUtilsWrapper + @Mock + lateinit var notificationsActionsWrapper: NotificationsActionsWrapper + + @Mock + lateinit var notificationsTableWrapper: NotificationsTableWrapper + private lateinit var viewModel: UnifiedCommentDetailsViewModel private val uiStates = mutableListOf() private val uiActionEvents = mutableListOf() private val snackbarMessages = mutableListOf() + private val moderatedStatuses = mutableListOf() private val site = SiteModel().apply { id = LOCAL_SITE_ID @@ -81,6 +95,7 @@ class UnifiedCommentDetailsViewModelTest : BaseUnitTest() { whenever(commentsStore.likeComment(eq(site), eq(REMOTE_COMMENT_ID), eq(null), any())) .thenReturn(successPayload()) whenever(dateTimeUtilsWrapper.javaDateToTimeSpan(any())).thenReturn("2 hours ago") + whenever(notificationsActionsWrapper.downloadNoteAndUpdateDB(any())).thenReturn(true) viewModel = UnifiedCommentDetailsViewModel( mainDispatcher = testDispatcher(), @@ -89,7 +104,9 @@ class UnifiedCommentDetailsViewModelTest : BaseUnitTest() { commentsStore = commentsStore, localCommentCacheUpdateHandler = localCommentCacheUpdateHandler, networkUtilsWrapper = networkUtilsWrapper, - dateTimeUtilsWrapper = dateTimeUtilsWrapper + dateTimeUtilsWrapper = dateTimeUtilsWrapper, + notificationsActionsWrapper = notificationsActionsWrapper, + notificationsTableWrapper = notificationsTableWrapper ) setupObservers() @@ -355,14 +372,94 @@ class UnifiedCommentDetailsViewModelTest : BaseUnitTest() { assertThat(uiStates.last().status).isEqualTo(APPROVED) } + @Test + fun `onEditClicked uses the note identifier in note mode`() = test { + viewModel.start(site, REMOTE_COMMENT_ID, NOTE_ID) + + viewModel.onEditClicked() + + val event = uiActionEvents.last() + assertThat((event as LaunchEditComment).commentIdentifier) + .isEqualTo(NotificationCommentIdentifier(NOTE_ID, REMOTE_COMMENT_ID)) + } + + @Test + fun `moderation in note mode refreshes the note and fires commentModerated`() = test { + viewModel.start(site, REMOTE_COMMENT_ID, NOTE_ID) + + viewModel.onApproveClicked() + + verify(notificationsActionsWrapper).downloadNoteAndUpdateDB(NOTE_ID) + assertThat(moderatedStatuses).containsExactly(UNAPPROVED) + } + + @Test + fun `failed moderation in note mode neither refreshes the note nor fires commentModerated`() = test { + whenever(commentsRsDataSource.updateStatus(eq(site), eq(REMOTE_COMMENT_ID), any())) + .thenReturn(RsResult.Error("nope")) + viewModel.start(site, REMOTE_COMMENT_ID, NOTE_ID) + + viewModel.onApproveClicked() + + verifyNoInteractions(notificationsActionsWrapper) + assertThat(moderatedStatuses).isEmpty() + } + + @Test + fun `moderation without a note does not touch the notification wrappers`() = test { + viewModel.start(site, REMOTE_COMMENT_ID) + + viewModel.onApproveClicked() + + verifyNoInteractions(notificationsActionsWrapper) + verifyNoInteractions(notificationsTableWrapper) + assertThat(moderatedStatuses).isEmpty() + } + + @Test + fun `like in note mode refreshes the note`() = test { + viewModel.start(site, REMOTE_COMMENT_ID, NOTE_ID) + + viewModel.onLikeClicked() + + verify(notificationsActionsWrapper).downloadNoteAndUpdateDB(NOTE_ID) + } + + @Test + fun `replying to an unapproved comment in note mode fires commentModerated with approved`() = test { + whenever(commentsRsDataSource.getComment(site, REMOTE_COMMENT_ID)).thenReturn(UNAPPROVED_RS_COMMENT) + viewModel.start(site, REMOTE_COMMENT_ID, NOTE_ID) + + viewModel.onReplyClicked("nice post") + + assertThat(moderatedStatuses).containsExactly(APPROVED) + } + + @Test + fun `like state falls back to the note when the comment has no cache row`() = test { + val note = mock() + whenever(note.hasLikedComment()).thenReturn(true) + whenever(notificationsTableWrapper.getNoteById(NOTE_ID)).thenReturn(note) + whenever(commentsStore.getCommentByLocalSiteAndRemoteId(LOCAL_SITE_ID, REMOTE_COMMENT_ID)) + .thenReturn(emptyList()) + whenever(commentsRsDataSource.fetchPostTitles(site, listOf(REMOTE_POST_ID))) + .thenReturn(emptyMap()) + + viewModel.start(site, REMOTE_COMMENT_ID, NOTE_ID) + + assertThat(uiStates.last().isLiked).isTrue + } + private fun setupObservers() { uiStates.clear() uiActionEvents.clear() snackbarMessages.clear() + moderatedStatuses.clear() viewModel.uiState.observeForever { uiStates.add(it) } viewModel.uiActionEvent.observeForever { it.applyIfNotHandled { uiActionEvents.add(this) } } viewModel.onSnackbarMessage.observeForever { it.applyIfNotHandled { snackbarMessages.add(this) } } + viewModel.commentModerated.observeForever { it.applyIfNotHandled { moderatedStatuses.add(this) } } } private fun successPayload() = CommentsActionPayload(CommentsActionData(emptyList(), 0)) @@ -374,6 +471,7 @@ class UnifiedCommentDetailsViewModelTest : BaseUnitTest() { private const val REMOTE_COMMENT_ID = 4321L private const val REMOTE_POST_ID = 99L private const val LOAD_DELAY_MS = 1000L + private const val NOTE_ID = "note_5555" private val RS_COMMENT = RsComment( remoteCommentId = REMOTE_COMMENT_ID, From e7e197b058e8af176917a6125956abce0d97156d Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Wed, 8 Jul 2026 06:21:53 -0400 Subject: [PATCH 18/20] Support notification args and result extras in the rs comment detail New fragment factory takes noteId, reply prefill and focus flags; the result intent accumulates the NOTE_MODERATE_* extras the notifications list uses to update the moderated note's row. Not yet reachable from the notifications flow. Co-Authored-By: Claude Fable 5 --- .../unified/UnifiedCommentDetailsFragment.kt | 66 +++++++++++++++++-- 1 file changed, 62 insertions(+), 4 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsFragment.kt index 15e62225be12..71710e84eb3c 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsFragment.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsFragment.kt @@ -37,11 +37,13 @@ import org.wordpress.android.fluxc.model.CommentStatus.UNAPPROVED import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.ui.CollapseFullScreenDialogFragment import org.wordpress.android.ui.CommentFullScreenDialogFragment +import org.wordpress.android.ui.ScrollableViewInitializedListener import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent.Close import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent.LaunchEditComment import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent.OpenPostInReader import org.wordpress.android.ui.comments.unified.CommentDetailsActionEvent.ReplySent import org.wordpress.android.ui.comments.unified.UnifiedCommentDetailsViewModel.CommentDetailsUiState +import org.wordpress.android.ui.notifications.NotificationsListFragment import org.wordpress.android.ui.pages.SnackbarMessageHolder import org.wordpress.android.ui.reader.ReaderActivityLauncher import org.wordpress.android.ui.suggestion.util.SuggestionServiceConnectionManager @@ -57,6 +59,7 @@ import org.wordpress.android.util.SnackbarItem.Info import org.wordpress.android.util.SnackbarSequencer import org.wordpress.android.util.ToastUtils import org.wordpress.android.util.WPLinkMovementMethod +import org.wordpress.android.util.extensions.focusAndShowKeyboard import org.wordpress.android.util.extensions.getColorFromAttribute import org.wordpress.android.util.extensions.getColorResIdFromAttribute import org.wordpress.android.util.extensions.getSerializableCompat @@ -89,8 +92,13 @@ class UnifiedCommentDetailsFragment : private lateinit var site: SiteModel private var remoteCommentId: Long = 0 + private var noteId: String? = null private var suggestionServiceConnectionManager: SuggestionServiceConnectionManager? = null + // The result reported to the launching screen, accumulated across observers: moderation adds + // the note extras the notifications list uses to update the moderated note's row. + private val resultIntent = Intent() + // The comment HTML currently rendered, so transient state changes (like/reply progress/status) // don't re-parse the body — which is expensive and resets the user's text selection. private var renderedCommentHtml: String? = null @@ -117,16 +125,20 @@ class UnifiedCommentDetailsFragment : site = requireNotNull(arguments?.getSerializableCompat(WordPress.SITE)) remoteCommentId = requireArguments().getLong(KEY_REMOTE_COMMENT_ID) + noteId = arguments?.getString(KEY_NOTE_ID) UnifiedCommentDetailsFragmentBinding.bind(view).apply { binding = this setupClickListeners() - layoutCommentBox.setupReplyBox() + layoutCommentBox.setupReplyBox(isFreshView = savedInstanceState == null) layoutCommentBox.setupSuggestions() setupObservers() + // Lets the notifications host lift its app bar with the comment's scroll position; + // the rs comments list host doesn't implement the listener, so this is a no-op there. + (activity as? ScrollableViewInitializedListener)?.onScrollableViewInitialized(scrollView.id) } - viewModel.start(site, remoteCommentId) + viewModel.start(site, remoteCommentId, noteId) } private fun UnifiedCommentDetailsFragmentBinding.setupClickListeners() { @@ -144,7 +156,7 @@ class UnifiedCommentDetailsFragment : if (SiteUtils.isAccessedViaWPComRest(site)) View.VISIBLE else View.GONE } - private fun ReaderIncludeCommentBoxBinding.setupReplyBox() { + private fun ReaderIncludeCommentBoxBinding.setupReplyBox(isFreshView: Boolean) { layoutContainer.visibility = View.VISIBLE editComment.initializeWithPrefix('@') editComment.doAfterTextChanged { btnSubmitReply.isEnabled = !it.isNullOrBlank() } @@ -154,6 +166,16 @@ class UnifiedCommentDetailsFragment : // application-password sites, which would collide drafts across sites. editComment.autoSaveTextHelper.uniqueId = "${site.id}-$remoteCommentId" editComment.autoSaveTextHelper.loadString(editComment) + // Prefill from the notification's inline-reply text, but never clobber an autosaved draft. + val prefill = arguments?.getString(KEY_PREFILL_REPLY_TEXT) + if (editComment.text.isNullOrEmpty() && !prefill.isNullOrEmpty()) { + editComment.setText(prefill) + } + // Opened via the notification's "reply" action: focus the reply field right away, like the + // legacy detail. Only on the first creation — not again after a rotation. + if (isFreshView && arguments?.getBoolean(KEY_FOCUS_REPLY_FIELD) == true) { + editComment.focusAndShowKeyboard() + } } private fun ReaderIncludeCommentBoxBinding.setupSuggestions() { @@ -238,7 +260,17 @@ class UnifiedCommentDetailsFragment : // Report the change to the launching screen (the rs comments list only refreshes when // the result says something actually changed). viewModel.commentChanged.observeEvent(viewLifecycleOwner) { - requireActivity().setResult(AppCompatActivity.RESULT_OK) + requireActivity().setResult(AppCompatActivity.RESULT_OK, resultIntent) + } + + // Note mode: attach the extras the notifications list uses to update the moderated + // note's row without waiting for its next refresh. + viewModel.commentModerated.observeEvent(viewLifecycleOwner) { status -> + noteId?.let { id -> + resultIntent.putExtra(NotificationsListFragment.NOTE_MODERATE_ID_EXTRA, id) + resultIntent.putExtra(NotificationsListFragment.NOTE_MODERATE_STATUS_EXTRA, status.toString()) + requireActivity().setResult(AppCompatActivity.RESULT_OK, resultIntent) + } } } @@ -491,7 +523,11 @@ class UnifiedCommentDetailsFragment : companion object { private const val KEY_REMOTE_COMMENT_ID = "key_remote_comment_id" + private const val KEY_NOTE_ID = "key_note_id" + private const val KEY_PREFILL_REPLY_TEXT = "key_prefill_reply_text" + private const val KEY_FOCUS_REPLY_FIELD = "key_focus_reply_field" + @JvmStatic fun newInstance(site: SiteModel, remoteCommentId: Long): UnifiedCommentDetailsFragment { return UnifiedCommentDetailsFragment().apply { arguments = Bundle().apply { @@ -500,5 +536,27 @@ class UnifiedCommentDetailsFragment : } } } + + /** + * Factory for the notifications host: [noteId] puts the screen in note mode (see + * [UnifiedCommentDetailsViewModel]), [prefillReplyText] carries the notification's + * inline-reply text and [focusReplyField] opens the keyboard on the reply field. + */ + @JvmStatic + fun newInstance( + site: SiteModel, + remoteCommentId: Long, + noteId: String, + prefillReplyText: String?, + focusReplyField: Boolean + ): UnifiedCommentDetailsFragment { + return newInstance(site, remoteCommentId).apply { + requireArguments().apply { + putString(KEY_NOTE_ID, noteId) + putString(KEY_PREFILL_REPLY_TEXT, prefillReplyText) + putBoolean(KEY_FOCUS_REPLY_FIELD, focusReplyField) + } + } + } } } From f0990edacfd0aecb5bac7599629382c1749578ef Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Wed, 8 Jul 2026 06:22:00 -0400 Subject: [PATCH 19/20] Serve comment notifications with the rs comment detail when gated NotificationsDetailActivity's pager now hosts the rs detail fragment for comment notes when the site resolves and the RS_UNIFIED_COMMENTS gate (renamed to shouldUseRsComments) is on; the legacy fragment stays as the fallback. Co-Authored-By: Claude Fable 5 --- .../android/ui/ActivityLauncher.java | 10 +++++--- .../NotificationsDetailActivity.java | 23 +++++++++++++++---- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/ActivityLauncher.java b/WordPress/src/main/java/org/wordpress/android/ui/ActivityLauncher.java index 26f8801c91cf..eec446804957 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/ActivityLauncher.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/ActivityLauncher.java @@ -745,7 +745,7 @@ public static void viewPageParentForResult(@NonNull Fragment fragment, @NonNull public static void viewUnifiedComments(Context context, SiteModel site) { Intent intent; - if (shouldUseRsCommentsList(context, site)) { + if (shouldUseRsComments(context, site)) { intent = CommentsRsListActivity.Companion.createIntent(context); } else { intent = new Intent(context, UnifiedCommentsActivity.class); @@ -755,9 +755,13 @@ public static void viewUnifiedComments(Context context, SiteModel site) { AnalyticsUtils.trackWithSiteDetails(AnalyticsTracker.Stat.OPENED_COMMENTS, site); } - private static boolean shouldUseRsCommentsList(@NonNull Context context, @NonNull SiteModel site) { + /** + * Whether the wordpress-rs comments screens (list, and the notification-hosted detail) should + * be used for {@code site} instead of the legacy (FluxC) ones. + */ + public static boolean shouldUseRsComments(@NonNull Context context, @NonNull SiteModel site) { // The rs client can only authenticate WP.com-accessed sites or self-hosted sites with an - // application password; everywhere else keep the legacy (FluxC) comments list. + // application password; everywhere else keep the legacy (FluxC) comments screens. if (!site.isUsingWpComRestApi() && !site.hasApplicationPassword()) { return false; } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsDetailActivity.java b/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsDetailActivity.java index 8ae0d432070c..f0254a11daa2 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsDetailActivity.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsDetailActivity.java @@ -40,6 +40,7 @@ import org.wordpress.android.ui.WPWebViewActivity; import org.wordpress.android.ui.comments.CommentActions; import org.wordpress.android.ui.comments.CommentDetailFragment; +import org.wordpress.android.ui.comments.unified.UnifiedCommentDetailsFragment; import org.wordpress.android.ui.engagement.EngagedPeopleListFragment; import org.wordpress.android.ui.engagement.ListScenarioUtils; import org.wordpress.android.ui.main.BaseAppCompatActivity; @@ -402,11 +403,23 @@ private Fragment createDetailFragmentForNote(@NonNull Note note) { // show comment detail for comment notifications boolean isInstantReply = getIntent().getBooleanExtra(NotificationsListFragment.NOTE_INSTANT_REPLY_EXTRA, false); - fragment = CommentDetailFragment.newInstance(note.getId(), - getIntent().getStringExtra(NotificationsListFragment.NOTE_PREFILLED_REPLY_EXTRA)); - - if (isInstantReply) { - ((CommentDetailFragment) fragment).enableShouldFocusReplyField(); + String prefilledReply = getIntent().getStringExtra(NotificationsListFragment.NOTE_PREFILLED_REPLY_EXTRA); + SiteModel site = mSiteStore.getSiteBySiteId(note.getSiteId()); + // The rs detail needs a real site from the SiteStore; the legacy fragment can fall + // back to a dummy WP.com site built from the note, so it stays the catch-all. + if (site != null && note.getCommentId() != 0 && ActivityLauncher.shouldUseRsComments(this, site)) { + fragment = UnifiedCommentDetailsFragment.newInstance( + site, + note.getCommentId(), + note.getId(), + prefilledReply, + isInstantReply + ); + } else { + fragment = CommentDetailFragment.newInstance(note.getId(), prefilledReply); + if (isInstantReply) { + ((CommentDetailFragment) fragment).enableShouldFocusReplyField(); + } } } else if (note.isAutomattcherType()) { // show reader post detail for automattchers about posts - note that comment From 0f4f9f36c5b63ddafa7034d5dfde015204bb441d Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Wed, 8 Jul 2026 09:33:13 -0400 Subject: [PATCH 20/20] Give the rs comment detail a surface background The notifications pager paints its ViewPager2 with a placeholder gray; the rs detail fragment's root was transparent, so that gray showed through inside a notification. Match the legacy comment detail and set colorSurface on the root. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/main/res/layout/unified_comment_details_fragment.xml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/WordPress/src/main/res/layout/unified_comment_details_fragment.xml b/WordPress/src/main/res/layout/unified_comment_details_fragment.xml index a059b80eca6d..a19675a4085d 100644 --- a/WordPress/src/main/res/layout/unified_comment_details_fragment.xml +++ b/WordPress/src/main/res/layout/unified_comment_details_fragment.xml @@ -3,7 +3,8 @@ xmlns:tools="http://schemas.android.com/tools" android:id="@+id/coordinator" android:layout_width="match_parent" - android:layout_height="match_parent"> + android:layout_height="match_parent" + android:background="?attr/colorSurface">