Skip to content

Conversation

@jaclync
Copy link
Contributor

@jaclync jaclync commented Jul 17, 2025

For WOOMOB-767

Just one review is required.

Why

For stores eligible for POS, when transitioning from eligibility check to items loading, both being async tasks, the spinner animation appears discontinuous during the transition.

How

This PR streamlines the animation by moving the eligibility state based UI from a separate view (entry point view) to the dashboard view with some refactoring on the view state. Other attempts on the loading UI animation side that did not work due to having 3 sets of animations using SwiftUI withAnimation which is reset when the view is redrawn (triggered when entering PointOfSaleDashboardView):

  • Updating IndefiniteCircularProgressViewStyle to only start the timer if there is no timer before, and disable invalidating the timer in onDisappear.
  • Extracting the animation states to be an ObservableObject owned by the entry point view and DI'ed to both loading use cases in the entry point view and dashboard view.
  • Replacing withAnimation entirely using CADisplayLink timer - I could not get it to match the previous animation 100%.

Description

This PR moves the eligibility state based UI from PointOfSaleEntryPointView to PointOfSaleDashboardView and refactors PointOfSaleEntryPointView view body to use a centralized ViewState approach with a view helper class, improving code maintainability and following the POS architecture guidelines.

Key Changes:

  1. Created PointOfSaleDashboardViewHelper: A stateless helper that centralizes ViewState determination logic
  2. ViewState enum: includes all cases in the top-level view body with associated values for the ineligible and error cases
  3. Streamlined view logic: Replaced nested conditional checks with a clean switch statement
  4. Test coverage: Added test cases for PointOfSaleDashboardViewHelper
  5. Some adjustments to the ineligible UI presentation are required now that it's moved to PointOfSaleDashboardView, like updating the floating control visibility and adding the frame width to fit to parent width

Architecture Benefits:

  • Single source of truth: All view state logic centralized in the helper
  • Better separation of concerns: Helper handles state determination, view handles rendering
  • Improved testability: Helper is easily unit tested with comprehensive coverage
  • Cleaner code structure: Replace nested conditionals with clear switch statements

Steps to reproduce

Prerequisite: the store is in an eligible country (US/UK) and is one fixable reason away from being eligible for POS (e.g. unsupported currency, POS feature switch disabled for WC version 10.0+)

  • Log in to the store in the prerequisite if needed
  • Tap on the POS tab --> after loading UI, ineligible UI should be shown. the floating control should not be shown
  • Fix the ineligible reason
  • In the app, tap Retry --> it should enter loading UI to fetch products, and to the products loaded state
  • Smoke test the items operations a bit, like searching for products/coupons to ensure that the states work as before
  • Exit POS
  • Tap on the POS tab --> the loading animation should be continuous before reaching the items loaded state

Testing information

Tested on iPad Pro 11in 3rd generation device, iOS 18.5:

  • Horizontal size class changes using split view (compact, regular)
  • Eligibility states:
    • Ineligible -> eligible after retry
    • Eligible
  • Priority ordering (size class > eligibility > container state)
  • Error

Screenshots

Ineligible -> eligible with split screen testing:

ineligible-to-eligible.mp4

Eligible where the animation is now continuous:

eligible.mp4

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

jaclync added 2 commits July 17, 2025 11:21
…to `PointOfSaleDashboardView` to enable continuous loading animation. UI state is determined by `PointOfSaleDashboardViewHelper`.
@jaclync jaclync added type: task An internally driven task. feature: POS labels Jul 17, 2025
@jaclync jaclync added this to the 22.9 milestone Jul 17, 2025
@jaclync jaclync changed the title Refactor PointOfSaleDashboardView to use centralized ViewState with helper [POS as a tab i2] Streamline loading UI animation when transitioning from eligibility check to items loading Jul 17, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jul 17, 2025

App Icon📲 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 NameWooCommerce iOS Prototype
Build Numberpr15922-dda7a33
Version22.8
Bundle IDcom.automattic.alpha.woocommerce
Commitdda7a33
Installation URL1vfdki03fik8g
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jaclync jaclync requested review from iamgabrielma and staskus July 17, 2025 17:18
@iamgabrielma iamgabrielma self-assigned this Jul 18, 2025
Copy link
Contributor

@iamgabrielma iamgabrielma left a comment

Choose a reason for hiding this comment

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

LGTM! 🚢

@available(iOS 17.0, *)
@Test func handleTap_when_attempt_to_add_duplicated_coupons_in_list_then_does_not_add_it_to_cart() async throws {
let aggregateModel = PointOfSaleAggregateModel(itemsController: MockPointOfSaleItemsController(),
let aggregateModel = PointOfSaleAggregateModel(entryPointController: POSEntryPointController(eligibilityChecker: MockPOSEligibilityChecker()),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think we can make a func makeAggregate() -> PointOfSaleAggregateModel helper in this test file, all cases seem to DI the same dependency into the SUT.

Copy link
Contributor Author

@jaclync jaclync Jul 18, 2025

Choose a reason for hiding this comment

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

When running the tests locally, I encountered memory crashes with/without the improvements in ea85627 sometimes:

#0	0x0000000104c88874 in __pthread_kill ()
#1	0x000000010437a2ec in pthread_kill ()
#2	0x0000000180171ea8 in abort ()
#3	0x00000001801f4450 in malloc_vreport ()
#4	0x00000001801f4610 in malloc_report ()
#5	0x00000001801c3708 in ___BUG_IN_CLIENT_OF_LIBMALLOC_POINTER_BEING_FREED_WAS_NOT_ALLOCATED ()
#6	0x00000001803a7270 in -[__NSArrayM insertObject:atIndex:] ()
#7	0x000000010280d46c in +[WPAnalytics registerTracker:] at /Users/jaclynchen/Developer/woocommerce-ios/Modules/Sources/WordPressSharedObjC/Analytics/WPAnalytics.m:25
#8	0x000000010ba93674 in WooAnalytics.init(analyticsProvider:) at /Users/jaclynchen/Developer/woocommerce-ios/WooCommerce/Classes/Analytics/WooAnalytics.swift:43
#9	0x000000010ba9358c in WooAnalytics.__allocating_init(analyticsProvider:) ()
#10	0x0000000133db65fc in default argument 7 of makePointOfSaleAggregateModel(entryPointController:itemsController:purchasableItemsSearchController:couponsController:couponsSearchController:cardPresentPaymentService:orderController:analytics:collectOrderPaymentAnalyticsTracker:searchHistoryService:popularPurchasableItemsController:barcodeScanService:soundPlayer:paymentState:) at /Users/jaclynchen/Developer/woocommerce-ios/WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift:1020
#11	0x0000000133dba018 in PointOfSaleAggregateModelTests.OrderStageTests.addMoreToCart_moves_to_building_order_stage() ()
#12	0x0000000133db9c58 in static PointOfSaleAggregateModelTests.OrderStageTests.$s16WooCommerceTests025PointOfSaleAggregateModelC0V010OrderStageC0V43addMoreToCart_moves_to_building_order_stage4TestfMp_9Z7114aedefMu_@Sendable () at /var/folders/dq/w2cq_v314913sdrjxwl5881c0000gn/T/swift-generated-sources/@__swiftmacro_16WooCommerceTests025PointOfSaleAggregateModelC0V010OrderStageC0V43addMoreToCart_moves_to_building_order_stage4TestfMp_.swift:7

But I didn't see any previous mentions from MGS, nor from CI failures. If it happens on CI, I'll revert the commit and we can look into improvements in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All good on CI, merging now.

@available(iOS 17.0, *)
@Test func handleTap_when_attempt_to_add_duplicated_coupons_in_list_then_does_not_add_it_to_cart() async throws {
let aggregateModel = PointOfSaleAggregateModel(itemsController: MockPointOfSaleItemsController(),
let aggregateModel = PointOfSaleAggregateModel(entryPointController: POSEntryPointController(eligibilityChecker: MockPOSEligibilityChecker()),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think we can make a func makeAggregate() -> PointOfSaleAggregateModel helper in this test file, all cases seem to DI the same dependency into the SUT.

Comment on lines +134 to +144
@Test(arguments: [
PointOfSaleErrorState.errorOnLoadingProducts(),
PointOfSaleErrorState.errorOnLoadingVariations(),
PointOfSaleErrorState.errorOnLoadingCoupons(),
PointOfSaleErrorState.errorCouponsDisabled,
PointOfSaleErrorState.errorOnLoadingProductsNextPage(),
PointOfSaleErrorState.errorOnLoadingVariationsNextPage(),
PointOfSaleErrorState.errorOnLoadingCouponsNextPage(),
PointOfSaleErrorState.errorOnRefreshingCoupons(),
PointOfSaleErrorState.errorOnEnablingCoupons()
])
Copy link
Contributor

Choose a reason for hiding this comment

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

nice SwiftTesting 💯

// MARK: - Horizontal Size Class Tests

@available(iOS 17.0, *)
@Test func determineViewState_when_horizontalSizeClass_is_compact_returns_unsupportedWidth() async throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting to see that with this approach we can now unit test unsupported widths, nice!

case .coupons(search: true):
await posModel.couponsSearchController.loadItems(base: .root)
}
switch viewState {
Copy link
Contributor

Choose a reason for hiding this comment

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

much cleaner view hierarchy 💯

@jaclync jaclync merged commit af9b54d into trunk Jul 18, 2025
13 checks passed
@jaclync jaclync deleted the feat/WOOMOB-767-streamline-indefinite-circular-animation branch July 18, 2025 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: POS type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants