[CIAB] BookingListViewModel refactoring and some fixes #14780

hichamboushaba · 2025-10-17T17:06:35Z

Description

This PR has three main changes:

Implement the approach discussed previously to make the ViewModel more reactive, and avoid reading the fetch parameters from StateFlow#value.
Fix an issue with pull-to-refresh and search (check the comments for more details)
Fix an issue with the WCSearchView composable (check the comments for more details)

Steps to reproduce

Use a CIAB site that has multiple bookings, multiple pages.
Open bookings list.

Testing information

Test changing tabs, searching.
Test changing the sorting order, and confirm it shows the refresh status.
Check for any regressions.

The tests that have been performed

The above.

I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

…ernally

wpmobilebot · 2025-10-17T17:11:42Z

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`edcbf72`
	Direct Download	`woocommerce-wear-prototype-build-pr14780-edcbf72.apk`

hichamboushaba · 2025-10-17T17:12:38Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/list/BookingListHandler.kt

+            if (searchQuery != previousQuery) {
+                searchResults.value = emptyList()
+            }


This is the main part of the second change mentioned in the description, it fixes an issue where pull-to-refresh would result in displaying the Empty state during the fetch. I added two unit tests to confirm the behavior too.

Check the below recordings to see the diffference:

Before After

Screen_recording_20251017_181115.mp4

Screen_recording_20251017_181007.mp4

hichamboushaba · 2025-10-17T17:14:31Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/compose/component/TextFields.kt

+    // Keep TextFieldValue in sync with external value
+    val textFieldValue = textFieldValueState.copy(text = value)


Without this, when the value is changed externally (for example by tapping on the clear button), the update is not reflected on the text field.
This line is copied from the BasicTextField String implementation.

Before After

Screen_recording_20251017_181130.mp4

Screen_recording_20251017_181025.mp4

wpmobilebot · 2025-10-17T17:26:38Z

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

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`edcbf72`
	Direct Download	`woocommerce-prototype-build-pr14780-edcbf72.apk`

codecov-commenter · 2025-10-17T17:49:18Z

Codecov Report

❌ Patch coverage is 77.58621% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.06%. Comparing base (95fd6da) to head (edcbf72).
⚠️ Report is 72 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...e/android/ui/bookings/list/BookingListViewModel.kt	82.60%	2 Missing and 6 partials ⚠️
...rce/android/ui/bookings/list/BookingListHandler.kt	70.00%	2 Missing and 1 partial ⚠️
...ommerce/android/ui/compose/component/TextFields.kt	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #14780      +/-   ##
============================================
+ Coverage     38.04%   38.06%   +0.01%     
- Complexity     9997    10003       +6     
============================================
  Files          2120     2120              
  Lines        119445   119477      +32     
  Branches      16254    16261       +7     
============================================
+ Hits          45447    45480      +33     
+ Misses        69412    69409       -3     
- Partials       4586     4588       +2

☔ View full report in Codecov by Sentry.
📢 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.

Copilot

Pull Request Overview

This PR refactors the BookingListViewModel to implement a more reactive approach that avoids reading fetch parameters from StateFlow values, while fixing issues with pull-to-refresh functionality during search operations and WCSearchView text field synchronization.

Replaced imperative fetch parameter reading with reactive parameter combinations using Flow operators
Fixed search result handling to properly clear results when search query changes and maintain previous results during refresh
Corrected text field value synchronization in WCSearchView component

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
BookingListViewModel.kt	Refactored to use reactive parameter combining and added refresh trigger mechanism
BookingListHandler.kt	Fixed search result handling logic and pagination for search operations
TextFields.kt	Fixed text field value synchronization issue in WCSearchField component
BookingListHandlerTest.kt	Added comprehensive tests for search refresh and query change scenarios

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-20T07:53:38Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/list/BookingListViewModel.kt

+                refreshTrigger.collect {
+                    fetchBookings(
+                        initialLoadingState = BookingListLoadingState.Refreshing,
+                        fetchParams = fetchParams
+                    )
+                }


The refreshTrigger.collect call inside collectLatest creates a nested collection that will never complete. This should be handled using combine or merge to properly react to refresh triggers alongside parameter changes.

I think we are good here, but I tend to agree, I never like a collect inside another collect.

Yes we are good, the collectLatest means that we will cancel the nested collect on each new emission.

The above patterns allows us to retrigger the fetching with the same parameters without reverting to reading the values from the Flows like how we did before, so I think it fits our need well.

I ended up getting rid of the nested collection, as it caused a minor issue before: pull-to-refresh action needed to wait for any pending fetch to finish before triggerring, with the last change in the commit 090a148, we will cancel pending fetches on pull-to-refresh, which matches the previous behavior.

Copilot · 2025-10-20T07:53:39Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/list/BookingListViewModel.kt

+    private suspend fun fetchBookings(
+        initialLoadingState: BookingListLoadingState,
+        fetchParams: FetchParams,
+    ) {


The fetchBookings function is now suspend but is called from within collectLatest which already runs in a coroutine context. Consider making this a regular function and launching coroutines internally where needed, or ensure proper structured concurrency.

AdamGrzybkowski

There is an issue when I try to pull to refresh with a search field activated but empty:

Screen_recording_20251020_133633.mp4

AdamGrzybkowski · 2025-10-20T11:38:53Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/list/BookingListHandler.kt

            )
        } else {
-            searchResults
+            searchResults.take(page * PAGE_SIZE)


Do we need that?

searchResults returns a List of bookings, not single bookings so I'm not sure why we take page * PAGE_SIZE 🤔

Given we don't clear the results now when after refresh if the query is not modified, in case more than a single page was previously fetched, this will mean that the "load more" login won't work as expected without this, as we won't reach the view that triggers it.
This is similar to what we do with the DB observation too.

Does this clarifies it?

To be honest, no 😅

This is similar to what we do with the DB observation too.

DB uses that value to limit the nr of entries returned. Here we are taking a specific number of values (list#1, list#2...) so those seem like different things 🤔

I'm testing the code and if I remove the take operator it it behaves in the same way. I can reach the view that triggers the next page load.

Something like this would make more sense searchResults.map { it.take(page * PAGE_SIZE) } but I'm not sure we need that at all. When the pull to refresh is triggered the page is reset to 1, so we for the short moment when the refresh is happening we would show less items than before triggering it 🤔

searchResults.map { it.take(page * PAGE_SIZE) }

That's what I thought I was doing 🤦 😅

I'm testing the code and if I remove the take operator it it behaves in the same way. I can reach the view that triggers the next page load.

The question here, is what's the end of the list? If we keep all the items, then the end of the list is further more than our current page, this breaks the pagination logic, because when the first page is fetched, we'll then get to this line while will remove the items you are seeing at the end of list, and then the list will jump above, and won't be a good experience.
Again, this logic is similar to what we do with DB, because we clear the DB after the fetch, we ask the DB to return only the number of items that fits in our pages count.

Let me know if this still doesn't clarify it 🙂, if you agree with the above, then I'll fix the logic to apply take on the list of items and not the Flow.

The question here, is what's the end of the list? If we keep all the items, then the end of the list is further more than our current page, this breaks the pagination logic, because when the first page is fetched, we'll then get to this line while will remove the items you are seeing at the end of list, and then the list will jump above, and won't be a good experience.

When you pull to refresh you are already at the top, so there's no visible jump in the list as there no visible change.

Anyway

Let me know if this still doesn't clarify it 🙂, if you agree with the above, then I'll fix the logic to apply take on the list of items and not the Flow.

That sounds good! My main issue was the .take on the Flow, which was breaking my brain, because I though I'm missing something obvious.

When you pull to refresh you are already at the top, so there's no visible jump in the list as there no visible change.

But the user can scroll down when the fetch is ongoing, and if the fetch takes long, then this would be noticeable.

Thanks @AdamGrzybkowski for catching the issue, I made the change in the last commit edcbf72

AdamGrzybkowski · 2025-10-20T11:41:38Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/list/BookingListViewModel.kt

+                refreshTrigger.collect {
+                    fetchBookings(
+                        initialLoadingState = BookingListLoadingState.Refreshing,
+                        fetchParams = fetchParams
+                    )
+                }


I think we are good here, but I tend to agree, I never like a collect inside another collect.

AdamGrzybkowski · 2025-10-20T11:46:08Z

Hmm the issue above might not be related to this PR 🤔

hichamboushaba · 2025-10-20T11:48:53Z

@AdamGrzybkowski let me take a look, even if unrelated to the PR, it would be good to investigate it and fix it in this PR.

To solve an issue where the loading state changes are conflated, resulting in a stuck refresh indicator in the screen.

Also fixes an issue where pull-to-refresh action needed to wait for current fetch to finish before triggering.

hichamboushaba · 2025-10-20T13:29:01Z

@AdamGrzybkowski the issue you found should be fixed by the commit 6fe7f69. The cause of the issue was that StateFlow would conflate values emitted quickly before the collector had a chance to observe them, so with our logic to return results right away when clearing the search query, we were setting the loading state to Idle right after setting it to Refreshing, so the Compose code didn't get any update to isRefreshing.

AdamGrzybkowski · 2025-10-20T14:20:52Z

@AdamGrzybkowski the issue you found should be fixed by the commit 6fe7f69. The cause of the issue was that StateFlow would conflate values emitted quickly before the collector had a chance to observe them, so with our logic to return results right away when clearing the search query, we were setting the loading state to Idle right after setting it to Refreshing, so the Compose code didn't get any update to isRefreshing.

This is fixed, thanks!

AdamGrzybkowski

hichamboushaba added 4 commits October 17, 2025 17:03

Refactor BookingListViewModel logic to make it more reactive

c8be29a

Make sure search results are not reset when refreshing

5b7c598

Add unit tests

7c0fb08

Fix issue with not updating the search view when query is changed ext…

5982f5d

…ernally

hichamboushaba added type: task An internally driven task. feature: CIAB Mobile Bookings MVP labels Oct 17, 2025

hichamboushaba commented Oct 17, 2025

View reviewed changes

hichamboushaba requested a review from AdamGrzybkowski October 17, 2025 17:15

hichamboushaba marked this pull request as ready for review October 17, 2025 17:15

hichamboushaba requested a review from JorgeMucientes October 17, 2025 17:15

hichamboushaba added this to the 23.6 milestone Oct 17, 2025

hichamboushaba requested a review from Copilot October 20, 2025 07:53

Copilot AI reviewed Oct 20, 2025

View reviewed changes

AdamGrzybkowski self-assigned this Oct 20, 2025

AdamGrzybkowski requested changes Oct 20, 2025

View reviewed changes

hichamboushaba added 2 commits October 20, 2025 14:11

Ensure we use some delay when the query gets emptied

6fe7f69

To solve an issue where the loading state changes are conflated, resulting in a stuck refresh indicator in the screen.

Refactor refreshing logic to avoid nesting flow collection

090a148

Also fixes an issue where pull-to-refresh action needed to wait for current fetch to finish before triggering.

hichamboushaba requested a review from AdamGrzybkowski October 20, 2025 13:30

Fix issue with take operator

edcbf72

AdamGrzybkowski approved these changes Oct 21, 2025

View reviewed changes

AdamGrzybkowski merged commit b1afe27 into trunk Oct 21, 2025
16 checks passed

AdamGrzybkowski deleted the CIAB/make-BookingListViewModel-reactive branch October 21, 2025 11:50

		// Keep TextFieldValue in sync with external value
		val textFieldValue = textFieldValueState.copy(text = value)

Uh oh!

[CIAB] BookingListViewModel refactoring and some fixes #14780

[CIAB] BookingListViewModel refactoring and some fixes #14780

Uh oh!

Conversation

hichamboushaba commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps to reproduce

Testing information

The tests that have been performed

Uh oh!

wpmobilebot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wpmobilebot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

AdamGrzybkowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamGrzybkowski commented Oct 20, 2025

Uh oh!

hichamboushaba commented Oct 20, 2025

Uh oh!

hichamboushaba commented Oct 20, 2025

Uh oh!

AdamGrzybkowski commented Oct 20, 2025

Uh oh!

AdamGrzybkowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hichamboushaba commented Oct 17, 2025 •

edited

Loading

wpmobilebot commented Oct 17, 2025 •

edited

Loading

wpmobilebot commented Oct 17, 2025 •

edited

Loading

codecov-commenter commented Oct 17, 2025 •

edited

Loading