From 2fc6f16668224aa63243e5467f66a37b6d398aad Mon Sep 17 00:00:00 2001 From: Nick Bradbury Date: Thu, 2 Jul 2026 12:22:09 -0400 Subject: [PATCH 01/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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/16] 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