[PM-38507] feat: Add Binding<Bool> initializer to ExpandableHeaderView#2583
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2583 +/- ##
==========================================
- Coverage 87.22% 86.65% -0.57%
==========================================
Files 1919 2137 +218
Lines 173520 187486 +13966
==========================================
+ Hits 151346 162460 +11114
- Misses 22174 25026 +2852 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f9e4cd5 to
a51442e
Compare
7132959 to
4dad4b4
Compare
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the additive change to Code Review DetailsNo findings. |
4dad4b4 to
3f4713a
Compare
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. 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. 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.
3f4713a to
6149c22
Compare
|
Thanks @morganzellers-bw |
🎟️ Tracking
PM-38507
📔 Objective
ExpandableHeaderViewstores its expand/collapse state in an internal@State, so the preference resets whenever the view is recreated. The upcoming BWPM vault-list section headers need that preference to survive app relaunch, which requires caller-owned storage.This adds a second initializer accepting
isExpanded: Binding<Bool>so callers can drive expansion from their own store. The existing(title:count:content:)initializer is unchanged, so Authenticator's call site is untouched.