Migrate the Domain Suggestions Screen to wordpress-rs - #23255
Merged
Conversation
Collaborator
Generated by 🚫 Danger |
Contributor
|
|
Contributor
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #23255 +/- ##
==========================================
+ Coverage 37.76% 37.81% +0.04%
==========================================
Files 2347 2347
Lines 128318 128360 +42
Branches 17760 17770 +10
==========================================
+ Hits 48463 48542 +79
+ Misses 75899 75861 -38
- Partials 3956 3957 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
oguzkocer
marked this pull request as ready for review
August 24, 2026 01:24
Contributor
Author
Contributor
|
It looks good, but there are a couple of points I think it is worth it to have a look:
|
adalpari
approved these changes
Aug 25, 2026
adalpari
left a comment
adalpari
left a comment
Contributor
There was a problem hiding this comment.
Tested the changes and they look good as well.
So, other than my previous comment LGTM!
Fetch domain products and suggestions through `WpComApiClient` instead of `ProductsStore` and the `Dispatcher`/EventBus pair. Suspend calls remove the need for the `@Subscribe` handler and the dispatcher registration. Changes: - Swap `ProductsStore` and `Dispatcher` for `WpComApiClientProvider` and `AccountStore` - Map the `empty_results` rejection to an empty success, as FluxC's `SiteRestClient.suggestDomains` did - Carry the API's message into `ListState.Error`, and log failures with `toLogErrorString` - Retarget `Product.isOnSale` at the wordpress-rs `Product`, whose `saleCost` holds hundredths of the currency unit rather than a decimal amount - Extract `buildSuggestionsParams`, and cover the sale, error and superseded query paths
Extract `buildProductsParams` so the filter can be asserted. `request` takes an opaque lambda, so a verify on the client counts calls without pinning which endpoint each one reached: swapping `ProductTypeFilter.Domains` for another filter left every test green while no suggestion would carry a sale price. Changes: - Extract `buildProductsParams` and assert it selects `Domains` - Rename `domain products are fetched only at first start` to name the repeat-start guarantee its call count actually covers
The observers closed over the `onDomainSelectedEvents` and `suggestionStates` fields, which `createViewModel` reassigns. A test that builds a second view model leaves the first one's observers writing into the second one's lists.
The fallback to `R.string.domain_suggestions_fetch_error` happens in `DomainSuggestionsFragment`, which this test does not exercise. It asserts that `ListState.Error` carries no message.
`suggestions` is a plain field written from the search field, the debouncer and the response handler. FluxC pinned the handler to the main thread through `@Subscribe(threadMode = ThreadMode.MAIN)`; the coroutine that replaced it runs on `BG_THREAD`, so a response landing while `fetchSuggestions` is still running can be overwritten by the stale state that thread holds. `postValue` then coalesces the two, leaving the spinner up with no error reported. A request that fails without reaching the network, as it does with no connection, is fast enough to make the overlap likely. Changes: - Inject the `UI_THREAD` dispatcher and run `onDomainSuggestionsFetched` on it
The screen reported every failure as "Domain suggestions couldn't be loaded", including the one the user can act on. wordpress-rs distinguishes a request that never reached the network, so an offline search now says to check the connection. Changes: - Set `errorMessageResId` to `error_network_connection` when the result is a `DeviceIsOfflineError` - Prefer `errorMessageResId` over `errorMessage` in the fragment's toast
Typing the first character hides the intro block and swaps the search icon for a spinner. `animateLayoutChanges` animated the resulting bounds change, which is what makes the screen jump and the keyboard flicker while the first results load. The views around the header now snap rather than animating. #23247 drops the attribute from the site creation search, where the same animation was taking focus off the search field.
A failed search with nothing already on screen showed two messages at once: the empty view said no domains were found while a toast said the suggestions could not be loaded. The empty view is attached to the list through `setEmptyView`, so it appears for any empty list including the one an error leaves behind. The failure now goes into the empty view when the list is empty, with the reason as its subtitle, and falls back to a toast only when there are still results on screen to keep.
The layout shipped the search icon `gone` and the progress bar visible, the inverse of the `tools:visibility` on each. The fragment corrects both the first time `suggestions` is assigned, so any path that reaches the screen without issuing a search leaves the spinner running with nothing behind it.
The list answers the query in the search field, and `ListState.Error` is built from the previous state's data. Rejecting a query therefore left the previous query's results on screen beneath the new text, so searching `...` after a real search showed that search's domains as though they answered it. The results are already hidden while a request is in flight, so this stops a failure restoring them rather than introducing a new behaviour. With the list always empty on failure, the empty view carries every error and the toast goes away, which also stops the longer API messages being truncated.
Emptying the field left the last result on screen, so an error explaining how a query has to be written stayed up after that query was deleted. Clearing now returns the list to `Init`, the state the screen opens in, and the fragment hides the list and its empty view there as it does while loading. The default search that follows an emptied field is unchanged, so the site's own suggestions still come back when they were not already showing.
A site with no name gives `initializeDefaultSuggestions` nothing to search for, on open and again whenever the field is emptied. The blank query went to the API, which rejects it as `invalid_query`, so emptying the field replaced the error it was meant to clear with an identical one. A blank query now leaves the list in `Init` without a request, which is the resting state the field's placeholder already implies.
Restoring the view state sets the search field's text, and `TextView` reports that as a change even when the text is empty, so a rotation tells the view model the query was cleared. Clearing reset the list to `Init`, and no fetch follows when the query is already the site name, so the retained suggestions were replaced by a blank screen with no way back but typing. Emptying the field now only resets the list when the query has moved off the site name, which is the case the reset exists for.
`fetchSuggestions` clears the selection at the end, and the blank-query guard returns before reaching it. Emptying the field on a site with no name left the Select button enabled over a list that had just been cleared, holding a domain the user could no longer see, and tapping it created a cart. The reset also moves above the request, so every write this thread makes to `suggestions` is done before a response can arrive on another one. The comment on the response handler claimed that as an effect of running on the main thread, which it never was — `fetchSuggestions` runs on the debouncer's scheduler, so the two threads overlapped either way.
… earlier implementation
`AccountStore.accessToken` is typed nullable but reads `""` when signed out, and is only null between an in-process sign out and the next launch. A null check therefore passed in the case it was written for and the screen sent `Authorization: Bearer ` for the server to reject, which arrived as the same generic failure as a dropped connection. In the case it did catch it threw inside `launch`, where nothing handles it. `getOrCreateClient` now returns null for either shape, matching `FetchAllDomainsUseCase`, and both requests report it instead: the product fetch logs and carries on, and the search shows the error the fragment already has a string for.
`suggestions` was read-modify-written from three threads: the debouncer's scheduler in `fetchSuggestions`, the main thread from the row taps and the response handler, and `BG_THREAD` from `showLoadingButton` during cart creation. Tapping Select as a response landed could interleave two of those cycles, dropping the fetched list or leaving rows enabled mid-purchase. `fetchSuggestions` and `showLoadingButton` now run on the main thread with the rest. The query is still read on the debouncer thread and passed in, so it remains the one the search was scheduled for rather than whatever the field holds by the time the main thread gets there.
oguzkocer
force-pushed
the
integrate-wordpress-rs-domain-suggestions-vm
branch
from
August 25, 2026 22:09
5de0684 to
977af14
Compare
Contributor
Author
|
@adalpari Thank you for the review.
Since the PR is approved, I'll land it. But if you have any thoughts about these 2 commits, please let me know and I'll either address them in #23247 or open a new PR. |
oguzkocer
enabled auto-merge (squash)
August 25, 2026 22:17
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Moves
DomainSuggestionsViewModeloff FluxC. Domain products and suggestions now come fromWpComApiClient.products().list()andWpComApiClient.domains().suggestions()rather thanProductsStoreand anewSuggestDomainsActiondispatch, so the@Subscribehandler, the EventBus registration and theDispatcherdependency all go away.CreateCartUseCasestays on FluxC; it is shared withPurchaseDomainViewModelandSiteCreationProgressViewModel, so it moves separately.Behaviour is meant to match trunk. The pieces worth checking:
empty_resultsis not a failure. The API reports "no domains for that search" as an HTTP error, andSiteRestClient.suggestDomainsturned it back into a successful empty list. The migration does the same, so the list empties instead of raising a toast.invalid_querystill carries the API's message, as on trunk. This screen treats it differently from the site creation search, where FluxC gave it its own empty state.Product.saleCostas a decimal amount and tested> 0.0. wordpress-rs types it asDecimal2, which holds hundredths of the currency unit —700is7.00— so the test is> 0L. The two agree, where a null check would not.searchQueryis captured before the request rather than read after it, which preserves the check trunk made againstOnSuggestedDomains.query.The logging changed along the way. Both failure paths now report through
toLogErrorString, and the product fetch names products rather than "site domains", which it never fetched. Its success path logged the entire product catalogue throughAppLog.d; that is gone.The failure path, and the layout animation
Several departures from a straight port, all found while testing the failure paths. They were rough enough on this screen to be worth fixing here rather than filing.
An offline failure says so. Every failure produced "Domain suggestions couldn't be loaded", including the one the user can do something about — FluxC's no-connection error carried an empty message (
BaseRequest.java:88-92) and the fragment fell back to that string. wordpress-rs reportsDeviceIsOfflineErrorseparately, so that case now setserrorMessageResIdtoerror_network_connection, "Check your network connection and try again".A failed search clears the list, and is reported once.
ListState.Erroris built from the previous state's data, so a rejected query left the previous query's results on screen underneath the new text — searching...after a real search showed that search's domains as though they answered it. Results are already hidden while a request is in flight, so this stops a failure restoring them rather than introducing a new behaviour, and it matchesNewDomainSearchViewModel, whose exclusiveUiStatereplaces the list the moment a search starts.With the list always empty on failure, the empty view carries every error — title, and the reason as its subtitle — and the toast is gone. Previously the two could contradict each other, the empty view reporting no matches while a toast reported a failure, and the toast truncated the longer API messages.
Emptying the field clears the results for the same reason: an error explaining how a query has to be written outlived the query it described. The list returns to
Init, which the fragment renders as a blank content area rather than an empty list, and the selected domain is dropped so the Select button cannot stay armed over it. This applies only when the query has moved off the site name — restoring the view state after a configuration change reports the emptied field as a change too, and the retained suggestions have to survive that.A blank query is not searched for.
initializeDefaultSuggestionssearches forsite.name, which runs on open and again whenever the field is emptied. A site with no name made that a blank query, the API rejects a blank query asinvalid_query, and so emptying the field replaced the error it was meant to clear with an identical one. A blank query now settles onInitwithout a request.The screen starts at rest. The layout declared the search icon
goneand the progress bar visible — the inverse of thetools:visibilityon each, which say what the resting state was meant to be. The fragment only corrects them oncesuggestionsis first assigned, so a screen that opens without issuing a search sits on a spinner with a blank list behind it. Both defaults now match the not-loading state.The implicit layout animation is gone. Typing the first character hides the intro block and swaps the search icon for a spinner, and
animateLayoutChangesanimated the resulting bounds change, which is what made the screen jump while the first results loaded. The views around the header now snap. #23247 drops the same attribute from the site creation search, where the animation was also taking focus off the search field.Testing instructions
Reach the screen: My Site → Domains → the domain call to action at the bottom (Claim your free domain, Add your domain or Get your domain, depending on the site's plan and credit).
Stop before completing a purchase. Selecting a domain on this screen creates a shopping cart.
Typing is not interrupted:
A normal search:
coolsite..wordpress.comsuggestion is among them.A query with no matches:
zcharacters.A query the API refuses, after one that worked:
...Rotation:
No connection:
Typing quickly:
Screenshots