POS Bookings: Address PR review feedback for date picker performance by staskus · Pull Request #16717 · woocommerce/woocommerce-ios

staskus · 2026-02-24T07:31:02Z

Description

Addresses code review feedback from #16701.

Changes:

Remove duplicated fetchLocalBookingsFromStorage function
Extract complex cache clear strategy ternary to a named function
Rename prefetchDateIfNeeded to prefetchDate (always re-fetches, only guards against duplicate in-flight requests)
Remove custom accessibility label from POSBookingRowView (combined children default is sufficient)
Move fetchLocalBookings() default from protocol extension to search strategy explicitly
Move syncBookings() call from POSBookingsModel init to POSBookingListController init
Improve readability: rename showsLoadingWithItems to showsCachedDataWhileLoading, restructure setLocalDataOrLoadingState() to showCachedBookingsOrLoadingIndicator(), add comments clarifying the sync-then-read and two-phase loading patterns
Pagination fixes:
- Gate loadNextBookings() until first-page loading has finished and hasMoreItems is true. We need to wait since we need to know if there's actually anything to paginate.
- Discard stale paginated results when strategy context changes mid-fetch.

Test Steps

Verify bookings list loads, date navigation works, prefetching works as before
Try paginate quickly after opening the view, switch dates while paginating
Verify search bookings flow still works

Pagination

Quick paginate while the first page is still loading

Quick.Case.mov

Pagination only after the first page has loaded

Slow.Case.mov

Pagination + Date switches

Switching.Case.mov

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Replace with the existing fetchLocalBookings() protocol method which has the same implementation. Addresses PR review feedback.

Replace the complex ternary with a cacheClearStrategy(for:) method that clearly documents the logic. Addresses PR review feedback.

The method always re-fetches data for the date (intentionally naive to keep data fresh), so 'ifNeeded' was misleading. The only guard is against duplicate in-flight requests. Addresses PR review feedback.

The combined children accessibility element already provides sufficient context. The custom label was redundant and made announcements longer without adding new information. Addresses PR review feedback.

Instead of providing a default empty implementation in the protocol extension, add it explicitly to POSSearchBookingListFetchStrategy and other conformers. This makes it clearer that search doesn't use local storage, and forces new strategy implementations to consider this method. Addresses PR review feedback.

…Controller init

Rename showsLoadingWithItems to showsCachedDataWhileLoading to clarify that it controls whether cached data is displayed during a remote fetch. Rename and restructure setLocalDataOrLoadingState() to showCachedBookingsOrLoadingIndicator() with early returns and a doc comment explaining the three branches. Add comments to the strategy's sync-then-read pattern and the controller's two-phase loading flow. Addresses PR review feedback about code intent being hard to follow.

wpmobilebot · 2026-02-24T07:41:49Z

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

	App Name	WooCommerce iOS Prototype
	Build Number	`pr16717-cb6153b`
	Version	`24.1`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`cb6153b`
	Installation URL	3e08kci3lhf90

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

joshheald

Looks great, a few tiny things I spotted, but nothing very urgent. Thanks for the improvements.

Pagination – I noticed that if we scroll quickly to the bottom, the loading row doesn't show until we finish loading the first page. It does trigger the pagination without further scrolling, but it's difficult to spot.

WDYT about these two options:

Optimistically add the pagination row even if we're not actually loading it yet, or
Scroll to the loading row when we add it.

I prefer the first as we may mess up the user's scroll position with the second... but I recognise this is quite picky for our (probably) temporary solution, so no worries if the answer is "third, leave it as is."

joshheald · 2026-02-24T16:35:26Z

Modules/Sources/PointOfSale/Controllers/POSBookingListController.swift

    func clearSearchBookings()
 }

+@MainActor


There are good arguments that any @Observable should be @MainActor.

joshheald · 2026-02-24T16:38:01Z

Modules/Sources/PointOfSale/Controllers/POSBookingListController.swift

+        guard paginationTracker.hasNextPage,
+              case .loaded(_, let hasMoreItems) = bookingsViewState, hasMoreItems else { return }
+
+        loadTask = Task {


Interesting, I didn't notice that we didn't put this in loadTask last time I reviewed. Was that just an error, previously, or is it to do with the pagination fixes?

joshheald · 2026-02-24T16:41:10Z

Modules/Sources/PointOfSale/Presentation/Bookings/POSBookingListView.swift

            InfiniteScrollView(
                triggerDeterminer: infiniteScrollTriggerDeterminer,
                loadMore: {
-                    guard case .loaded(_, let hasMoreItems) = bookingsViewState, hasMoreItems else { return }


Didn't notice this before but definitely seems better to take this logic out of the view 👍

joshheald · 2026-02-24T16:42:35Z

Modules/Sources/Yosemite/PointOfSale/BookingList/POSBookingListFetchStrategy.swift

                cacheClearStrategy: cacheClearStrategy
            )
-            let bookings = await fetchLocalBookingsFromStorage()
+            guard let filters else { return PagedItems(items: [], hasMorePages: hasMorePages, totalItems: nil) }


Suggested change

guard let filters else { return PagedItems(items: [], hasMorePages: hasMorePages, totalItems: nil) }

guard let filters else {

return PagedItems(items: [], hasMorePages: hasMorePages, totalItems: nil)

}

staskus added 7 commits February 24, 2026 07:03

Remove duplicated fetchLocalBookingsFromStorage function

fb81994

Replace with the existing fetchLocalBookings() protocol method which has the same implementation. Addresses PR review feedback.

Extract cache clear strategy ternary to a named function

9ee8eb8

Replace the complex ternary with a cacheClearStrategy(for:) method that clearly documents the logic. Addresses PR review feedback.

Rename prefetchDateIfNeeded to prefetchDate

f5d7fc3

The method always re-fetches data for the date (intentionally naive to keep data fresh), so 'ifNeeded' was misleading. The only guard is against duplicate in-flight requests. Addresses PR review feedback.

Remove custom accessibility label from POSBookingRowView

fb77ba9

The combined children accessibility element already provides sufficient context. The custom label was redundant and made announcements longer without adding new information. Addresses PR review feedback.

Move syncBookings() call from POSBookingsModel init to POSBookingList…

50acc84

…Controller init

staskus added this to the 24.2 milestone Feb 24, 2026

staskus added type: task An internally driven task. feature: POS labels Feb 24, 2026

Fix bookings pagination handling

cb6153b

staskus requested a review from joshheald February 24, 2026 15:30

staskus marked this pull request as ready for review February 24, 2026 15:31

joshheald approved these changes Feb 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

POS Bookings: Address PR review feedback for date picker performance#16717

POS Bookings: Address PR review feedback for date picker performance#16717
staskus wants to merge 8 commits intotrunkfrom
woomob-2267-pos-bookings-date-picker-performance-2

staskus commented Feb 24, 2026 •

edited

Loading

Uh oh!

wpmobilebot commented Feb 24, 2026 •

edited

Loading

Uh oh!

joshheald left a comment

Uh oh!

joshheald Feb 24, 2026

Uh oh!

joshheald Feb 24, 2026

Uh oh!

joshheald Feb 24, 2026

Uh oh!

joshheald Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

staskus commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test Steps

Pagination

Quick paginate while the first page is still loading

Pagination only after the first page has loaded

Pagination + Date switches

Uh oh!

wpmobilebot commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshheald left a comment

Choose a reason for hiding this comment

Uh oh!

joshheald Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

joshheald Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

joshheald Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

joshheald Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

staskus commented Feb 24, 2026 •

edited

Loading

wpmobilebot commented Feb 24, 2026 •

edited

Loading