[PM-35398] feat: Add Binding<Bool> initializer to ExpandableHeaderView#2583
Draft
SaintPatrck wants to merge 2 commits into
Draft
Conversation
Adds an additive `isExpanded: Binding<Bool>` initializer to `ExpandableHeaderView` so callers whose expand/collapse preference must survive view recreation (e.g., persisted across app launches in PR 6c) can drive the view from caller-owned storage. The original zero-ceremony `ExpandableHeaderView(title:count:content:)` initializer is preserved unchanged, so Authenticator's existing `ItemListView` call site compiles without modification. ## Storage pattern Chose option (c) from the brief's "cleanest approach" guidance: dual storage on a single type. The struct carries both an `@State private var internalIsExpanded` (used when no binding is supplied) and an `externalIsExpanded: Binding<Bool>?` (set by the binding init). A computed `isExpanded: Binding<Bool>` resolves to whichever storage the initializer populated. This: - Avoids the brittle `State(initialValue:).projectedValue` assign-in- init trick, which loses SwiftUI's state-tracking cell. - Avoids a public wrapper-view shim (approach a), which would expose additional API surface and leak an implementation type into call sites. - Keeps `body` on a single `@Binding` read path. - Preserves the `ExpandableHeaderView<Content>` identity for both initializers — no subtype or factory-function redirection needed. Authenticator's call site in `AuthenticatorShared/UI/Vault/ItemList/ItemList/ItemListView.swift:275` remains unchanged and still resolves to the no-binding initializer. ## Tests Adds `ExpandableHeaderView+ViewInspectorTests.swift` covering: - `test_init_withoutBinding_startsExpanded` — no-binding init renders child content on first render (expansion state defaults to true). - `test_init_withBinding_reflectsBindingState` — binding init reads the initial value from the caller's binding (true ⇒ content visible, false ⇒ content hidden). - `test_init_withBinding_updatesThroughBinding` — tapping the header writes the toggled value back through the caller-supplied binding. All three tests pass against BitwardenKit on iPhone 17 / iOS 26.1.
…nd-trip test) Applies four follow-up fixes from the PR 6b architecture and code reviews. - I-1 (arch): Document the SwiftUI identity invariant on both initializers. A given call site must commit to one initializer variant; switching between them at runtime is undefined because SwiftUI preserves the view's identity across renders and reuses the internal `@State` cell, silently abandoning whichever storage is no longer populated. - I-3 (arch): Name the default expansion state (`true`) as part of the no-binding initializer's public contract, and redirect callers who need "collapsed by default" behavior to the binding initializer with a pre-initialized `Binding<Bool>(false)`. -⚠️ 1 (code): Add a runtime round-trip test that exercises the header button tap repeatedly and asserts each tap writes back through the caller's binding. The existing single-tap test was already mediated by `ViewType.Button.tap()` (which invokes the action closure — including the `withAnimation` wrapper — through ViewInspector rather than calling the closure directly), so that test is kept; the new `test_init_withBinding_roundTripsAcrossMultipleToggles` guards against regressions where the `withAnimation` write stops propagating on subsequent toggles (e.g. a future refactor that captures `externalIsExpanded` by value). -⚠️ 2 (code): Remove the stale unused `@Previewable @swiftui.State var isExpanded = false` from the preview and split the preview into two canvases — "Internal state" exercising the no-binding init, "Caller-owned binding" exercising the new `Binding<Bool>` init with the previously-dead state variable now wired through the binding. All four tests pass on iPhone 17 / iOS 26.1. `AuthenticatorShared` still builds unchanged.
SaintPatrck
added
t:feature-app
github-actions
Bot
added
t:feature
and removed
t:feature-app
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## vault/pm-35398-ios-promote-expandable-header #2583 +/- ##
================================================================================
- Coverage 87.15% 86.08% -1.07%
================================================================================
Files 1887 2117 +230
Lines 166760 181612 +14852
================================================================================
+ Hits 145334 156349 +11015
- Misses 21426 25263 +3837 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
PM-35398
Stacked on #2582.
📔 Objective
ExpandableHeaderViewbacks its expand/collapse state with an internal@State, which means the user's preference resets whenever the view is recreated. For the upcoming BWPM vault-list section headers, we want that preference to survive app relaunch — that requires caller-owned storage.Add a second initializer that accepts
isExpanded: Binding<Bool>so callers can drive expansion from their own persistence store. The existing(title:count:content:)initializer is preserved unchanged so Authenticator's call site is untouched.Storage-pattern rationale — why a dual-storage struct with a computed
isExpanded: Binding<Bool>rather than a wrapper view orState(initialValue:).projectedValueassign-in-init — is in the feat commit body.📸 Screenshots
N/A — no call site uses the new initializer yet; Authenticator's existing render path is unchanged.