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