Skip to content

Add paging to the new pages parent picker#23000

Merged
nbradbury merged 8 commits into
trunkfrom
new-pages-parent-picker
Jun 18, 2026
Merged

Add paging to the new pages parent picker#23000
nbradbury merged 8 commits into
trunkfrom
new-pages-parent-picker

Conversation

@nbradbury

@nbradbury nbradbury commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Description

The "Set Parent" picker in the new (wp-rs) pages list previously only offered pages that were already loaded into the tabs. On large sites this meant you couldn't choose a parent that hadn't been paged in yet, and its search field only filtered the loaded candidates.

Testing instructions

Requires a site with the experimental pages list enabled and enough published pages to paginate (more than ~20).

Parent picker paging:

  1. Open the Pages list and tap a page's overflow menu → Set parent.
  2. Scroll to the bottom of the candidate list.
  • Verify more pages load automatically as you reach the end.

Server-side search:

  1. In the Set parent sheet, type the title of a page that is NOT in the first loaded batch.
  • Verify it appears in the results (previously it would not).
  1. Clear the search field.
  • Verify the "Top level" entry reappears and the full paged list returns.

Selecting a parent:

  1. Pick a parent (or "Top level") and confirm.
  • Verify the page's parent updates and the list refreshes.

The "Set Parent" picker previously only offered pages already loaded into
the tabs, so on large sites you couldn't choose a parent that hadn't been
paged in yet. Give the picker its own observable collection so it pages
through and server-searches the full list of eligible published pages.

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

dangermattic commented Jun 17, 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 Jun 17, 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
Versionpr23000-779d895
Build Number1497
Application IDorg.wordpress.android.prealpha
Commit779d895
Installation URL53qfbh0gqdup8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

wpmobilebot commented Jun 17, 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
Versionpr23000-779d895
Build Number1497
Application IDcom.jetpack.android.prealpha
Commit779d895
Installation URL2pk6se180si28
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

Copy link
Copy Markdown
Contributor

🤖 Build Failure Analysis

This build has failures. Claude has analyzed them - check the build annotations for details.

Merge the two early-return state guards in onLoadMoreParents into one so
the function stays under detekt's return-count limit. Behavior-preserving.

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

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 8.28729% with 166 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.42%. Comparing base (2c13628) to head (779d895).

Files with missing lines Patch % Lines
...rdpress/android/ui/pagesrs/PagesRsListViewModel.kt 13.76% 87 Missing and 7 partials ⚠️
...roid/ui/pagesrs/screens/PageRsParentPickerSheet.kt 0.00% 64 Missing ⚠️
.../wordpress/android/ui/pagesrs/PageRsListUiState.kt 0.00% 6 Missing ⚠️
...ss/android/ui/pagesrs/screens/PagesRsListScreen.kt 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #23000      +/-   ##
==========================================
- Coverage   37.46%   37.42%   -0.04%     
==========================================
  Files        2325     2325              
  Lines      125245   125399     +154     
  Branches    17171    17205      +34     
==========================================
+ Hits        46918    46929      +11     
- Misses      74520    74656     +136     
- Partials     3807     3814       +7     

☔ 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 nbradbury marked this pull request as ready for review June 17, 2026 16:36
@nbradbury nbradbury requested a review from adalpari June 17, 2026 16:36
@adalpari

Copy link
Copy Markdown
Contributor

Claude pointed out a medium-severity issue:

  1. Search race: overlapping init jobs are not cancelled (medium)
    onParentPickerQueryDebounced does closeParentPickerCollection() then initParentPickerCollection(), but closeParentPickerCollection() only closes the collection object — it does not cancel in-flight launchCollectionJob work (unlike clearCollections(), which cancels and recreates collectionsScope). On rapid query changes where two createParentPickerCollection IO calls overlap and complete out of order, parentPickerCollection = collection is a last-writer-wins assignment with no guarantee the survivor matches the latest query — the user can end up viewing stale search results. debounce(250ms) + distinctUntilChanged makes this unlikely but not impossible on slow networks.
    Consider giving the picker its own cancellable scope, or validating the assigned collection against the current query before publishing results.

closeParentPickerCollection only closed the collection object; it did not
cancel in-flight launchCollectionJob work. On rapid query changes two
createParentPickerCollection IO calls could overlap and complete out of
order, leaving parentPickerCollection assigned to a stale query's result.

Track the init coroutine in a Job and cancel it before launching a new
one. A cancelled job hits the existing CancellationException cleanup
(closing the half-built collection) and never reaches the assignment, so
the field always reflects the latest query.

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

Copy link
Copy Markdown
Contributor Author

Claude pointed out a medium-severity issue:

Thanks! Addressed in 174b931.

@adalpari

Copy link
Copy Markdown
Contributor

Not a blocker, but the search results blink. I found a situation where it started to blink for like a second.

Screen_recording_20260618_142543.mp4

@adalpari

Copy link
Copy Markdown
Contributor

Is this expected to happen?

2026-06-18 14:26:03.073  9555-9555  WordPress-PAGES         com.jetpack.android.prealpha         E  Failed to load more parents
                                                                                                    uniffi.wp_mobile.FetchException$StaleLoadMore: 
                                                                                                    	at uniffi.wp_mobile.FfiConverterTypeFetchError.read(wp_mobile.kt:11311)
                                                                                                    	at uniffi.wp_mobile.FfiConverterTypeFetchError.read(wp_mobile.kt:11300)
                                                                                                    	at uniffi.wp_mobile.FfiConverter$DefaultImpls.liftFromRustBuffer(wp_mobile.kt:270)
                                                                                                    	at uniffi.wp_mobile.FfiConverterRustBuffer$DefaultImpls.liftFromRustBuffer(wp_mobile.kt:286)
                                                                                                    	at uniffi.wp_mobile.FfiConverterTypeFetchError.liftFromRustBuffer(wp_mobile.kt:11300)
                                                                                                    	at uniffi.wp_mobile.FfiConverterTypeFetchError.liftFromRustBuffer(wp_mobile.kt:11300)
                                                                                                    	at uniffi.wp_mobile.FfiConverterRustBuffer$DefaultImpls.lift(wp_mobile.kt:287)
                                                                                                    	at uniffi.wp_mobile.FfiConverterTypeFetchError.lift(wp_mobile.kt:11300)
                                                                                                    	at uniffi.wp_mobile.FfiConverterTypeFetchError.lift(wp_mobile.kt:11300)
                                                                                                    	at uniffi.wp_mobile.FetchException$ErrorHandler.lift(wp_mobile.kt:11291)
                                                                                                    	at uniffi.wp_mobile.FetchException$ErrorHandler.lift(wp_mobile.kt:11290)
                                                                                                    	at uniffi.wp_mobile.Wp_mobileKt.uniffiCheckCallStatus(wp_mobile.kt:354)
                                                                                                    	at uniffi.wp_mobile.Wp_mobileKt.access$uniffiCheckCallStatus(wp_mobile.kt:1)
                                                                                                    	at uniffi.wp_mobile.Wp_mobileKt.uniffiRustCallAsync(wp_mobile.kt:13233)
                                                                                                    	at uniffi.wp_mobile.Wp_mobileKt$uniffiRustCallAsync$1.invokeSuspend(Unknown Source:19)
                                                                                                    	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:34)
                                                                                                    	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:100)
                                                                                                    	at kotlinx.coroutines.internal.LimitedDispatcher$Worker.run(LimitedDispatcher.kt:124)
                                                                                                    	at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:89)
                                                                                                    	at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:586)
                                                                                                    	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:798)
                                                                                                    	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:717)
                                                                                                    	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:704)

nbradbury and others added 3 commits June 18, 2026 08:45
Two parent-picker fixes from review:

- loadParentPickerItems published every intermediate loadItems() result,
  including the transient empty/partial sets emitted during a refresh,
  flipping the sheet to the spinner/"no results" state and back on each
  one. Skip publishing an empty list while a load is in progress so the
  list only updates once real results arrive (or the fetch finishes
  genuinely empty).

- onLoadMoreParents treated FetchException.StaleLoadMore as a user-facing
  error. It is a benign race (a refresh superseded the page request), so
  swallow it, clear the spinner, and skip the error snackbar.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The sheet sized itself to its content, so it grew and shrank as the
content region swapped between the spinner, the "no results" message and
the variable-length candidate list during search. Pin the sheet to a
fixed fraction of the screen height and host the content in a weighted
box so it always fills the same space.

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

Copy link
Copy Markdown
Contributor Author

Is this expected to happen?

Per Claude:

FetchException.StaleLoadMore is a benign race. So: yes, it’s expected to occur occasionally, 
but it shouldn’t have been treated as a user-facing error.

Fixed in 5a164bd

Not a blocker, but the search results blink.

I see what you mean. The bottom sheet resizes during search, first to show an empty list while the search results are being fetched, then again when search results are returned. I addressed this in 8ad34cc by making the sheet a fixed size.

@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.

Thank you for the changes, LGTM!

@nbradbury nbradbury merged commit 967f3c7 into trunk Jun 18, 2026
23 checks passed
@nbradbury nbradbury deleted the new-pages-parent-picker branch June 18, 2026 14:11
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