Skip to content

Convert the unified comment detail to Compose - #23078

Merged
nbradbury merged 34 commits into
trunkfrom
feature/rs-comment-detail-compose
Jul 10, 2026
Merged

Convert the unified comment detail to Compose#23078
nbradbury merged 34 commits into
trunkfrom
feature/rs-comment-detail-compose

Conversation

@nbradbury

@nbradbury nbradbury commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Description

Converts the unified (wordpress-rs) comment detail screen from XML/View Binding to a pure-Compose UI. The ViewModel is unchanged.

Testing instructions

Requires the RS Unified Comments experimental flag (Me → Experimental Features).

Comment detail from the rs comments list (WP.com site):

  1. Open Comments from My Site and tap a comment
  • Avatar, author, date, status badge, post title and HTML body (tappable links, embedded images, smilies) render correctly
  1. Toggle approve/unapprove, spam, like; open the more menu and try edit, trash (confirm dialog), copy link, share link; on a trashed/spammed comment, delete permanently (confirm dialog)
  • Spam/trash/delete close the screen; the list reflects each change

Reply box:

  1. Type a reply including @
  • Mention suggestions appear and insert on tap
  1. Send the reply; also try the expand affordance and send from the full-screen editor
  • Field clears, keyboard hides, success snackbar shows; text/cursor survive expand/collapse

Note mode:

  1. Open a comment notification (also via the notification's reply action)
  • Reply prefill/auto-focus work; moderating updates the notification row on return
  1. Rotate the device with a reply typed and a dialog open
  • Text and dialogs survive rotation

Self-hosted site:

  • Like button hidden, no mention suggestions, everything else works

nbradbury and others added 25 commits July 2, 2026 12:22
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 <noreply@anthropic.com>
…rience

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…g and SnackbarMessage

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
- 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) <noreply@anthropic.com>
- 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) <noreply@anthropic.com>
- 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
Long-press a comment to enter selection mode, then tap rows to build the
selection. The top bar swaps to a contextual action bar offering the same
per-tab actions as the legacy list's action mode (approve/unapprove/spam/
not-spam/trash/restore/delete), with confirmation dialogs for trash and
delete-permanently and COMMENT_BATCH_* analytics.

Moderation runs the rs writes in parallel, aggregates any failures into a
single snackbar, then refreshes all initialized tabs, since moderated
comments move between them. Selection clears on tab change and after each
batch action.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Search icon in the top bar opens an inline query field (like the rs posts
list): queries are debounced 250ms with a 3-character minimum, keep the
active tab's status filter, and clear back to the normal tab content when
search closes. Selection mode is cleared when search opens or closes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…list-search

# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListActivity.kt
#	WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListUiState.kt
#	WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListViewModel.kt
#	WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt
#	WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListScreen.kt
#	WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt
#	WordPress/src/main/res/values/strings.xml
- Track and cancel in-flight load-more jobs in clearTabs(): a survivor could
  land on a reset page-generation counter and append its stale (unfiltered)
  page into fresh search results, corrupting the cursor chain
- Don't resurrect a cleared tab from the stale-load-more discard branch; the
  ghost entry blocked initTab and left the tab shimmering forever
- Keep the tab row visible while searching: the comments endpoint accepts a
  single status per request (unlike posts), so a search is always scoped to
  one tab — visible tabs make that scope explicit and switchable
- Reset each tab's scroll position when search opens, changes query, or
  closes, so replaced content renders at the top instead of clamping to the
  old offset and tripping the load-more trigger
- Use trimmed query lengths for the debounce filter and the idle/searching
  thresholds, so whitespace-only queries can't fetch the unfiltered list
- Clear results on any below-minimum query (not just blank) so the previous
  query's rows can't flash as matches for the next one
- Guard initTab against fetching during search-idle (Activity recreation or
  a late pager settle would bypass the debounce and minimum length)
- Only clear the selection on a real tab change, not the settled-page
  re-emission after recreation, and request search focus once per open so a
  selection round-trip doesn't pop the keyboard

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Re-init the tab the user is looking at (lastTrackedTab, which follows
  every pager settle) instead of the tab captured at keystroke time; a tab
  switch inside the debounce window previously left the visible tab as a
  permanent shimmer with no fetch in flight. Removes the activeSearchTab
  field and the tab parameter from onSearchQueryChanged
- Trim and dedupe the query pipeline so whitespace-only edits neither
  restart the debounce nor refetch, and send the trimmed query to the API
- Clear displayed rows on any trimmed-query change, covering whole-string
  replacements (select-all + paste, keyboard suggestions, voice input)
  that never dip below the minimum length
- Rework the scroll reset: skip the initial emission so Activity
  recreation keeps the restored scroll positions, and skip lists already
  at the top to avoid pointless forced remeasures and fling cancellation
- Don't re-select failed batch-moderation ids into a search session opened
  while the batch ran; the untracked job outlives clearTabs and an
  invisible selection would swallow back presses and hijack row taps
- Consolidate the minimum-length policy into one isSearchable predicate
  exposed as isQuerySearchable, so the screen no longer re-derives it from
  the ViewModel's companion constant

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Guard the failed-batch re-selection with a clear-generation counter
  instead of the instantaneous search flag: a batch run inside an
  unchanged search keeps the retry re-selection (its failed rows are
  still displayed), while any clearTabs() during the batch — search
  opened, closed, or query changed — suppresses it, closing the
  invisible-selection hole when search was opened and closed mid-batch
- Unify the search-query policy in one flow: a shared trim+dedupe
  normalization feeds an immediate on-change clear and the debounced
  fetch, so the two halves can't drift; onSearchQueryChanged shrinks to
  a plain setter. The collector no longer re-clears, so a pager settle
  that already fetched the same query inside the debounce window keeps
  its results instead of being cancelled and refetched
- Track the pager's current tab in a dedicated field instead of reusing
  the analytics dedupe key, so tracking changes can't silently retarget
  the debounced search
- Render the blank search-idle screen (not the animated shimmer) for
  tabs cleared while awaiting the debounce, removing the per-keystroke
  results/shimmer flicker; shimmer now means an actual fetch in flight
- Pass isSearchActive/isQuerySearchable to the tab screen and derive
  the idle/searching split inside it, removing the representable-but-
  impossible flag combination
- Hoist the once-per-open focus request out of SearchTopBar, dropping
  its two ferry parameters

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Left behind when SearchTopBar's focus effect was hoisted to the caller.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When started with a noteId, the ViewModel refreshes the note DB after
every successful write, fires commentModerated for the notifications
result contract, edits via NotificationCommentIdentifier, and falls
back to Note.hasLikedComment() when the FluxC cache row is missing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
New fragment factory takes noteId, reply prefill and focus flags; the
result intent accumulates the NOTE_MODERATE_* extras the notifications
list uses to update the moderated note's row. Not yet reachable from
the notifications flow.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NotificationsDetailActivity's pager now hosts the rs detail fragment
for comment notes when the site resolves and the RS_UNIFIED_COMMENTS
gate (renamed to shouldUseRsComments) is on; the legacy fragment stays
as the fallback.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The notifications pager paints its ViewPager2 with a placeholder gray;
the rs detail fragment's root was transparent, so that gray showed
through inside a notification. Match the legacy comment detail and set
colorSurface on the root.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces the XML/View Binding UI of UnifiedCommentDetailsFragment with a
pure-Compose screen: content, action footer with dropdown more-menu,
confirm dialogs, snackbars, and a reply box with @-mention suggestions
and a full-screen editor replacing CollapseFullScreenDialogFragment.
The fragment is now a thin ComposeView host; the ViewModel is unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dangermattic

dangermattic commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator
2 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot

wpmobilebot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

App Icon📲 You can test the changes from this Pull Request in WordPress Android by scanning the QR code below to install the corresponding build.

App NameWordPress Android
Build TypeDebug
Versionpr23078-33e7b5e
Build Number1498
Application IDorg.wordpress.android.prealpha
Commit33e7b5e
Installation URL5labmqi0dla80
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

wpmobilebot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

App Icon📲 You can test the changes from this Pull Request in Jetpack Android by scanning the QR code below to install the corresponding build.

App NameJetpack Android
Build TypeDebug
Versionpr23078-33e7b5e
Build Number1498
Application IDcom.jetpack.android.prealpha
Commit33e7b5e
Installation URL46j6nm4ac9g68
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

nbradbury and others added 2 commits July 8, 2026 11:27
Follows the legacy displayHtmlComment pipeline: emoticon smilies become
unicode emoji first, then remaining <img> tags render as width-capped
block images via Coil, with link-aware text segments between them.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Moves CommentDetailsActions to its own file, rewrites findMentionToken
without loop jumps, and drops an unused import.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 37.38%. Comparing base (9a3dbf4) to head (33e7b5e).

Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #23078   +/-   ##
=======================================
  Coverage   37.37%   37.38%           
=======================================
  Files        2340     2340           
  Lines      126991   126990    -1     
  Branches    17507    17507           
=======================================
  Hits        47469    47469           
+ Misses      75639    75638    -1     
  Partials     3883     3883           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nbradbury and others added 2 commits July 8, 2026 14:20
…etail-compose

# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentDetailsFragment.kt
#	WordPress/src/main/res/layout/unified_comment_details_fragment.xml
Makes the fragment's action callbacks a single lazy instance, hoists
the full-screen editor's duplicated send-enabled expression, drops a
wrapper Column from the mention panel, and collapses the moderate
button's three near-identical branches into one call.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nbradbury
nbradbury marked this pull request as ready for review July 8, 2026 19:10
@nbradbury
nbradbury requested a review from adalpari July 8, 2026 19:10
nbradbury and others added 2 commits July 8, 2026 18:22
The More button's weight sat on the Box that anchors its dropdown, so
the button itself wrapped content and hugged the slot's start edge.
Fill the Box so the icon centres and the ripple covers the full slot,
matching the sibling action buttons.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@adalpari

adalpari commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Dialogs are not saved on device rotation. Expanded state is not sabed either, but that is maybe expected:

Screen_recording_20260709_160641.mp4

Claude also noted these points you might want to check:

  • Snackbar duration/dismiss-event fidelity (showSnackbar):
    • holder.onDismissAction(0) is always passed event code 0 (DISMISS_EVENT_SWIPE), whereas the legacy path forwarded the real event. It happens to be harmless for the holders this screen uses (their onDismissAction either is a no-op or unconditionally fires Close), but it's a latent trap if a future holder branches on the event code. Consider passing DISMISS_EVENT_ACTION when result == ActionPerformed.
    • snackbarHostState.showSnackbar(message, actionLabel) ignores holder.duration. With actionLabel == null (the only case here) Compose defaults to SnackbarDuration.Short, so these messages now display noticeably shorter than the previous LENGTH_LONG. Minor UX regression; pass an explicit duration if parity matters.
  • CommentHtmlBody uses coil AsyncImage directly while every avatar in the PR uses the project's RemoteImage wrapper. Worth confirming inline comment images don't need RemoteImage's handling (auth/photon/fallback); at minimum it's a consistency wrinkle.
  • PersistentEditTextDatabase lifecycle: it's an SQLiteOpenHelper created by lazy and never close()d. This matches how the legacy helper used it and the connection dies with the fragment, so it's not a real leak — just noting there's no explicit teardown.
  • Hardcoded typography/sizes: the composables use literal sp values (20.sp, 16.sp, 14.sp, 12.sp) where the deleted XML referenced dimension resources (text_sz_extra_large, etc.). Functionally equivalent, but it drops the shared dimens; consider MaterialTheme.typography for consistency with other Compose screens.
  • Test coverage: the trickiest new logic is pure and internal — findMentionToken, filterMentionSuggestions, applyMentionSuggestion, and splitCommentHtml/commentHtmlToAnnotatedString. Despite the unit-tests-exemption label, these are cheap, high-value unit tests (cursor edge cases, whitespace-only bodies, with/without matches, emoticon ordering) and would guard against regressions in the mention/HTML-splitting behavior.

nbradbury and others added 2 commits July 9, 2026 12:57
- Keep dialogs open across rotation: give the hosting ComposeView a stable
  id so the fragment restores its rememberSaveable dialog state on config
  change (was regenerating the id each onCreateView).
- Restore the snackbar's LENGTH_LONG display duration and forward the real
  dismiss event (ACTION vs TIMEOUT) to onDismissAction.
- Render inline comment-body images through the shared RemoteImage wrapper
  for consistency with the avatars; RemoteImage now takes optional
  contentScale/alignment and a nullable fallback.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Fire the snackbar's onDismissAction even when the view lifecycle cancels
  the coroutine (rotation), so the load-error snackbar still closes the
  screen like the legacy Snackbar callback did on view detach.
- Drop the Send IME action from the reply field so the enter key inserts a
  newline again (legacy textMultiLine parity); sending stays on the send
  button.
- Only show the mention suggestion panel while the reply field is focused,
  so a restored draft ending in an @-token doesn't pop the panel on open.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nbradbury

Copy link
Copy Markdown
Contributor Author

Claude also noted these points you might want to check:

@adalpari Apologies - I should've caught that bug. I fixed that along with the first three items for your Claude review. I also fixed a couple of things a re-review caught:

  • Pressing enter in a reply submits it so there's no way to add a new line to a reply
  • If you press @ in a reply to show the name list and back out, if you return to the comment the name list re-appears.

@adalpari adalpari left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I'm approving it as I consider it ready to go.

Claude keeps suggesting to include tests for some of the new added logic, but I leave that to your criteria.

@nbradbury
nbradbury merged commit 4378537 into trunk Jul 10, 2026
30 checks passed
@nbradbury
nbradbury deleted the feature/rs-comment-detail-compose branch July 10, 2026 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants