Skip to content

PM-38363: Add a loading ViewState to the PlanScreen#7080

Open
david-livefront wants to merge 1 commit into
mainfrom
PM-38363-plan-screen-loading-state
Open

PM-38363: Add a loading ViewState to the PlanScreen#7080
david-livefront wants to merge 1 commit into
mainfrom
PM-38363-plan-screen-loading-state

Conversation

@david-livefront

@david-livefront david-livefront commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

🎟️ Tracking

PM-38363

📔 Objective

This PR refactors the PlanViewModel and PlanScreen to have a top level loading and error state for the initial state.

📸 Screenshots

@david-livefront david-livefront requested a review from a team as a code owner June 19, 2026 18:13
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context labels Jun 19, 2026
@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR refactors PlanViewModel/PlanScreen to hoist loading and error handling into top-level ViewState variants (Loading, Error, and a new Content grouping) instead of overlay dialogs, and consolidates the four state-source flows into a single merge. Tests were updated comprehensively to match the new state model, including the new showsPremiumView/isSelfHosted fields. The refactor is clean and the screen still exposes a top-app-bar back affordance, so removing the error dialogs' "Close" button does not create a dead end.

Code Review Details
  • ❓ : Merged-flow gate (first { ... is Content }) blocks special-circumstance / status updates while the screen is in the Error state until a retry succeeds — confirm this is intended for the error path.
    • app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/premium/plan/PlanViewModel.kt:138

Note: handleClosePricingErrorClick / PlanAction.ClosePricingErrorClick appear to no longer have a UI entry point now that the pricing error is a full-screen Error state rather than a dialog with a Close button. Minor dead-code cleanup candidate, not blocking.

Comment on lines +138 to +141
.onEach {
// Wait until we are in the Content state so we can update everything appropriately
mutableStateFlow.first { it.viewState is PlanState.ViewState.Content }
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

QUESTION: This gate suspends the merged collector until the view state reaches Content. In the Error state (e.g. a premium user whose getSubscription() call failed), Content is never reached until the user taps "Try again" and it succeeds, so emissions from specialCircumstanceStateFlow / subscriptionStatusStateFlow are held the entire time the error is on screen.

Details

The updated PlanViewModelTest (around line 170) now seeds subscriptionResult = SUBSCRIPTION_SUCCESS_ACTIVE with the comment "The premium subscription must resolve so the screen reaches a Content state and the special-circumstance flow is processed" — confirming special circumstances are not processed until Content.

Concretely: if a StripePortal / PremiumCheckout special circumstance arrives while the screen is sitting in ViewState.Error, it won't be handled (and its specialCircumstance = null clear won't run) until a retry succeeds. Previously these four flows were collected independently, so they were processed regardless of view state.

Was gating these flows during Error (not just initial Loading) intentional, and is the "blocked until successful retry" behavior acceptable for the special-circumstance path? If so, it may be worth a test covering a special circumstance arriving during Error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app:authenticator Bitwarden Authenticator app context app:password-manager Bitwarden Password Manager app context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant