Skip to content

[Hack Week] Refactor connection checks to a list-driven architecture#15606

Open
hichamboushaba wants to merge 8 commits intohack-week/troubleshooting-screen-m4from
hack-week/troubleshooting-screen-m5-fixes
Open

[Hack Week] Refactor connection checks to a list-driven architecture#15606
hichamboushaba wants to merge 8 commits intohack-week/troubleshooting-screen-m4from
hack-week/troubleshooting-screen-m5-fixes

Conversation

@hichamboushaba
Copy link
Copy Markdown
Member

@hichamboushaba hichamboushaba commented Apr 2, 2026

Description

Note

Depends on #15597

This PR refactors the connection checks state machine to use a dynamic, list-driven architecture, significantly reducing boilerplate and making the code more scalable for future checks.

Key Changes:

  • Enum Abstraction: Replaced the verbose ConnectivityCheckCardData sealed class with a cleaner ConnectivityCheckType enum that encapsulates all UI and analytics constants (title, icon, suggestion, analytics value, operation name).
  • Unified State Flow: Consolidated individual StateFlows into a single checksFlow representing the sequential list of checks.
  • Dynamic UI Rendering: Updated ConnectivityToolScreen to iterate over the checks list dynamically instead of hardcoding each connectivity card sequentially.
  • Bug Fix (State Restoration Callbacks): Fixed a pre-existing issue where, after a process death and restoration, UI callbacks (such as "Retry connection" or "Read More") would stop working. This was previously caused by storing callback lambdas in state classes using @IgnoredOnParcel. The new architecture hoists these events to the ViewModel and passes them directly to the Composables, bypassing Parcelable limitations completely.
  • Test Coverage: Updated all ViewModel and UI tests to align with the new list-driven architecture.

Test Steps

  1. Navigate to the Troubleshoot Connection screen.
  2. Simulate a connection failure (e.g., using ApiFaker to timeout the orders or products endpoint).
  3. Verify the UI halts at the failed check and displays the error state correctly.
  4. Simulate process death (e.g., enable "Don't keep activities" in developer options, background the app, and bring it back to the foreground).
  5. Tap the "Retry connection" and "Read More" buttons.
  6. Verify that the callbacks trigger correctly after restoration (the check restarts or the troubleshooting web view opens).
  • 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.

@hichamboushaba hichamboushaba added type: technical debt Represents or solves tech debt of the project. feature: support Related to anything in the help & support section, including app logs and the Zendesk SDK. status: do not merge Dependent on another PR, ready for review but not ready for merge. labels Apr 2, 2026
@dangermattic
Copy link
Copy Markdown
Collaborator

dangermattic commented Apr 2, 2026

1 Error
🚫 This PR is tagged with status: do not merge label(s).
1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Copy Markdown
Collaborator

wpmobilebot commented Apr 2, 2026

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

App NameWooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Build Number736
Version24.4-rc-1
Application IDcom.woocommerce.android.prealpha
Commit1dcd4c8
Installation URL26taf6geghqkg
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.52632% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.62%. Comparing base (8352e3d) to head (1dcd4c8).

Files with missing lines Patch % Lines
...d/ui/connectivitytool/ConnectivityToolViewModel.kt 78.84% 6 Missing and 5 partials ⚠️
Additional details and impacted files
@@                            Coverage Diff                            @@
##             hack-week/troubleshooting-screen-m4   #15606      +/-   ##
=========================================================================
- Coverage                                  39.65%   39.62%   -0.04%     
- Complexity                                 11362    11363       +1     
=========================================================================
  Files                                       2263     2263              
  Lines                                     130803   130681     -122     
  Branches                                   18331    18321      -10     
=========================================================================
- Hits                                       51874    51781      -93     
+ Misses                                     73620    73605      -15     
+ Partials                                    5309     5295      -14     

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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the Troubleshoot Connection “connection checks” flow from a step-based/state-machine approach into a list-driven architecture, aiming to reduce boilerplate and make it easier to add/maintain checks while also fixing process-death callback restoration issues.

Changes:

  • Introduces ConnectivityCheckType + ConnectivityCheckCardData to centralize UI/analytics metadata and represent checks as a list.
  • Replaces multiple per-check StateFlows with a single checksFlow, and updates the ViewModel to execute checks sequentially from that list.
  • Updates Compose UI and unit tests to render/assert against the dynamic checks list.

Reviewed changes

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

File Description
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/connectivitytool/ConnectivityToolViewModel.kt Replaces state machine + per-check flows with a single list-backed checksFlow and sequential execution logic.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/connectivitytool/ConnectivityToolScreen.kt Renders connectivity cards by iterating the checks list and wires actions to ViewModel callbacks.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/connectivitytool/ConnectivityCheckCardData.kt Replaces sealed card data hierarchy with enum-driven check definitions and a simple parcelable card model.
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/connectivitytool/ConnectivityToolViewModelTest.kt Updates tests to assert against checks list and new completion logic.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 86 to 110
@@ -168,101 +98,55 @@ class ConnectivityToolViewModel @Inject constructor(
triggerEvent(Exit)
}

private fun handleRetryConnectionClick(step: ConnectivityCheckStep) {
when (step) {
InternetCheck -> internetCheckFlow.update { it.copy(connectivityCheckStatus = NotStarted) }
WPComCheck -> wpComCheckFlow.update { it.copy(connectivityCheckStatus = NotStarted) }
StoreCheck -> storeCheckFlow.update { it.copy(connectivityCheckStatus = NotStarted) }
StoreOrdersCheck -> ordersCheckFlow.update { it.copy(connectivityCheckStatus = NotStarted) }
StoreProductsCheck -> productsCheckFlow.update { it.copy(connectivityCheckStatus = NotStarted) }
Finished -> { /* No-op */ }
fun onRetryClicked(type: ConnectivityCheckType) {
checksFlow.update { checks ->
checks.map {
if (it.type == type) it.copy(status = NotStarted) else it
}
}
launch {
executeNextCheck()
}
stateMachine.update { step }
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

startConnectionChecks()/onRetryClicked launch executeNextCheck() without guarding against an already-running execution. If startConnectionChecks gets called again while a check is InProgress (e.g., Fragment view recreation on configuration change), you can end up running the same cold Flow twice, duplicating network requests and analytics events. Consider tracking a Job for the current execution and returning early (or cancelling/restarting) when it’s active, or otherwise ensuring only one executeNextCheck loop can run at a time.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The retry button is shown only when the current step is not running, so we are safe I think.

@hichamboushaba hichamboushaba marked this pull request as ready for review April 4, 2026 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: support Related to anything in the help & support section, including app logs and the Zendesk SDK. status: do not merge Dependent on another PR, ready for review but not ready for merge. type: technical debt Represents or solves tech debt of the project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants