From 7ebb12620620fff9ef7eb208ff6c5868a6cf985f Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Fri, 24 Apr 2026 17:32:21 -0400 Subject: [PATCH 1/2] [PM-35398] feat: Add Binding initializer to ExpandableHeaderView MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an additive `isExpanded: Binding` 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?` (set by the binding init). A computed `isExpanded: Binding` 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` 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. --- ...andableHeaderView+ViewInspectorTests.swift | 69 ++++++++++++++++++ .../Views/ExpandableHeaderView.swift | 72 ++++++++++++++++--- 2 files changed, 132 insertions(+), 9 deletions(-) create mode 100644 BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView+ViewInspectorTests.swift diff --git a/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView+ViewInspectorTests.swift b/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView+ViewInspectorTests.swift new file mode 100644 index 0000000000..51e11ed87e --- /dev/null +++ b/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView+ViewInspectorTests.swift @@ -0,0 +1,69 @@ +// swiftlint:disable:this file_name +import SwiftUI +import ViewInspector +import ViewInspectorTestHelpers +import XCTest + +@testable import BitwardenKit + +final class ExpandableHeaderViewTests: BitwardenTestCase { + // MARK: Tests + + /// `init(title:count:buttonAccessibilityIdentifier:content:)` starts in the expanded state. + @MainActor + func test_init_withoutBinding_startsExpanded() throws { + let subject = ExpandableHeaderView(title: "Title", count: 3) { + Text("Child") + } + + // The child content is rendered only when `isExpanded` is true, so its presence is a + // proxy for the initial expansion state. + XCTAssertNoThrow(try subject.inspect().find(text: "Child")) + } + + /// `init(title:count:buttonAccessibilityIdentifier:isExpanded:content:)` reflects the caller's + /// binding value on the first render. + @MainActor + func test_init_withBinding_reflectsBindingState() throws { + let collapsedSubject = ExpandableHeaderView( + title: "Title", + count: 3, + isExpanded: .constant(false), + ) { + Text("Hidden child") + } + XCTAssertThrowsError(try collapsedSubject.inspect().find(text: "Hidden child")) + + let expandedSubject = ExpandableHeaderView( + title: "Title", + count: 3, + isExpanded: .constant(true), + ) { + Text("Visible child") + } + XCTAssertNoThrow(try expandedSubject.inspect().find(text: "Visible child")) + } + + /// Tapping the header button with an external binding writes the toggled value back through + /// that binding so the caller's persisted storage is kept in sync. + @MainActor + func test_init_withBinding_updatesThroughBinding() throws { + var isExpanded = true + let binding = Binding(get: { isExpanded }, set: { isExpanded = $0 }) + let subject = ExpandableHeaderView( + title: "Title", + count: 3, + isExpanded: binding, + ) { + Text("Child") + } + + let button = try subject.inspect().find(ViewType.Button.self) + try button.tap() + + XCTAssertFalse( + isExpanded, + "Tapping the header should write the toggled value through the caller-supplied binding", + ) + } +} diff --git a/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView.swift b/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView.swift index e9f8d5955f..189e7271fe 100644 --- a/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView.swift +++ b/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView.swift @@ -5,6 +5,12 @@ import SwiftUI /// A wrapper around some content which can be expanded to show the content or collapsed to hide it. /// +/// Use ``init(title:count:buttonAccessibilityIdentifier:content:)`` when the view can own its +/// own expansion state (a fresh `@State` that resets on view recreation). Use +/// ``init(title:count:buttonAccessibilityIdentifier:isExpanded:content:)`` when the caller needs +/// to persist the expanded / collapsed preference across app launches or share it across views +/// (PM-35398). +/// public struct ExpandableHeaderView: View { // MARK: Properties @@ -14,25 +20,38 @@ public struct ExpandableHeaderView: View { /// The content that is shown when the section is expanded or hidden otherwise. let content: Content - /// A var to determine if the content in the section is expanded or collapsed. - @State private var isExpanded: Bool = true - /// A value indicating whether the expandable content is currently enabled or disabled. @Environment(\.isEnabled) var isEnabled: Bool /// The title of the Header button used to expand or collapse the content. let title: String - /// The count of items on the Content + /// The count of items on the Content. let count: Int + /// Expansion state ownership. When the caller supplies a `Binding` the view writes to + /// / reads from that binding directly; when the caller omits it, the view falls back to its + /// own `@State`-owned storage. Keeping both storages on the struct (rather than splitting + /// into two types) preserves the `ExpandableHeaderView(...)` initializer shape that + /// Authenticator relies on. + @State private var internalIsExpanded: Bool = true + + /// The caller-supplied binding, when present. `body` prefers this over `internalIsExpanded`. + private let externalIsExpanded: Binding? + + /// The unified binding used by `body` and `expandButton`. Resolves to `externalIsExpanded` + /// when the caller provided one, or to `$internalIsExpanded` otherwise. + private var isExpanded: Binding { + externalIsExpanded ?? $internalIsExpanded + } + // MARK: View public var body: some View { VStack(spacing: 8) { expandButton - if isExpanded { + if isExpanded.wrappedValue { content } } @@ -44,7 +63,7 @@ public struct ExpandableHeaderView: View { @ViewBuilder private var expandButton: some View { Button { withAnimation { - isExpanded.toggle() + isExpanded.wrappedValue.toggle() } } label: { HStack(spacing: 8) { @@ -52,7 +71,7 @@ public struct ExpandableHeaderView: View { SharedAsset.Icons.chevronDown16.swiftUIImage .imageStyle(.accessoryIcon16(scaleWithFont: true)) - .rotationEffect(isExpanded ? Angle(degrees: 180) : .zero) + .rotationEffect(isExpanded.wrappedValue ? Angle(degrees: 180) : .zero) } .multilineTextAlignment(.leading) .foregroundStyle(SharedAsset.Colors.textSecondary.swiftUIColor) @@ -65,24 +84,59 @@ public struct ExpandableHeaderView: View { // MARK: Initialization - /// Initialize an `ExpandableContent`. + /// Initialize an `ExpandableHeaderView` whose expansion state is owned by the view itself. + /// + /// The view creates an internal `@State` that starts in the expanded (`true`) state and is + /// reset whenever the view's identity changes. Use this initializer when the caller does not + /// need to persist or share the expansion state. + /// + /// - Parameters: + /// - title: The title of the button used to expand or collapse the content. + /// - count: The count of items on the Content. + /// - buttonAccessibilityIdentifier: The accessibility identifier for the button to expand or + /// collapse the content. + /// - content: The content that is shown when the section is expanded or hidden otherwise. + public init( + title: String, + count: Int, + buttonAccessibilityIdentifier: String = "ExpandSectionButton", + @ViewBuilder content: () -> Content, + ) { + self.buttonAccessibilityIdentifier = buttonAccessibilityIdentifier + self.content = content() + self.title = title + self.count = count + externalIsExpanded = nil + } + + /// Initialize an `ExpandableHeaderView` whose expansion state is owned by the caller. + /// + /// Use this initializer when the expanded / collapsed preference must survive view + /// recreation — typically because it is persisted to a store and rehydrated on view appear + /// (PM-35398). The caller is responsible for providing a `Binding` whose storage + /// outlives the view. /// /// - Parameters: /// - title: The title of the button used to expand or collapse the content. - /// - isExpanded: A binding to determine if the content in the section is expanded or collapsed. + /// - count: The count of items on the Content. /// - buttonAccessibilityIdentifier: The accessibility identifier for the button to expand or /// collapse the content. + /// - isExpanded: A binding that drives whether the content is currently expanded. The view + /// reads this value on every render and writes back through it when the user toggles + /// the header. /// - content: The content that is shown when the section is expanded or hidden otherwise. public init( title: String, count: Int, buttonAccessibilityIdentifier: String = "ExpandSectionButton", + isExpanded: Binding, @ViewBuilder content: () -> Content, ) { self.buttonAccessibilityIdentifier = buttonAccessibilityIdentifier self.content = content() self.title = title self.count = count + externalIsExpanded = isExpanded } } From 71329594ee89c796eeb65df33e15d81bf58f417c Mon Sep 17 00:00:00 2001 From: Patrick Honkonen Date: Fri, 24 Apr 2026 17:49:04 -0400 Subject: [PATCH 2/2] [PM-35398] docs: Apply PR 6b review fixes (DocC, preview hygiene, round-trip test) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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(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` 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. --- ...andableHeaderView+ViewInspectorTests.swift | 29 ++++++++++++ .../Views/ExpandableHeaderView.swift | 44 ++++++++++++++++--- 2 files changed, 68 insertions(+), 5 deletions(-) diff --git a/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView+ViewInspectorTests.swift b/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView+ViewInspectorTests.swift index 51e11ed87e..feae5306a4 100644 --- a/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView+ViewInspectorTests.swift +++ b/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView+ViewInspectorTests.swift @@ -66,4 +66,33 @@ final class ExpandableHeaderViewTests: BitwardenTestCase { "Tapping the header should write the toggled value through the caller-supplied binding", ) } + + /// Exercising the header repeatedly must keep the caller's binding coherent across toggles — + /// i.e. the computed `isExpanded: Binding` must continue to resolve to the + /// `externalIsExpanded` storage on every render, including renders that observe a previously + /// written value. This guards against regressions where the `withAnimation`-wrapped write + /// stops propagating after the first toggle (e.g. a future refactor to two disjoint storages + /// that accidentally captures `externalIsExpanded` by value). + @MainActor + func test_init_withBinding_roundTripsAcrossMultipleToggles() throws { + var isExpanded = true + let binding = Binding(get: { isExpanded }, set: { isExpanded = $0 }) + let subject = ExpandableHeaderView( + title: "Title", + count: 3, + isExpanded: binding, + ) { + Text("Child") + } + + for expectedAfterTap in [false, true, false] { + let button = try subject.inspect().find(ViewType.Button.self) + try button.tap() + XCTAssertEqual( + isExpanded, + expectedAfterTap, + "Each tap must write back through the caller-supplied binding; expected \(expectedAfterTap)", + ) + } + } } diff --git a/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView.swift b/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView.swift index 189e7271fe..0d096222ae 100644 --- a/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView.swift +++ b/BitwardenKit/UI/Platform/Application/Views/ExpandableHeaderView.swift @@ -86,9 +86,21 @@ public struct ExpandableHeaderView: View { /// Initialize an `ExpandableHeaderView` whose expansion state is owned by the view itself. /// - /// The view creates an internal `@State` that starts in the expanded (`true`) state and is - /// reset whenever the view's identity changes. Use this initializer when the caller does not - /// need to persist or share the expansion state. + /// The view creates an internal `@State` that is reset whenever the view's identity changes. + /// Use this initializer when the caller does not need to persist or share the expansion + /// state. + /// + /// The internal state defaults to expanded (`true`). This default is part of the public + /// contract — callers that need "collapsed by default" behavior should use + /// ``init(title:count:buttonAccessibilityIdentifier:isExpanded:content:)`` with a + /// pre-initialized `Binding` set to `false`. + /// + /// - Important: A given call site should commit to one initializer variant. Switching + /// between the no-binding and binding initializer at runtime (for example, inside a + /// conditional branch that flips its predicate) is undefined: both initializers produce + /// the same ``ExpandableHeaderView`` type, so SwiftUI preserves the view's identity and + /// reuses the internal `@State` cell, but whichever storage is no longer populated on the + /// new render will be silently abandoned. /// /// - Parameters: /// - title: The title of the button used to expand or collapse the content. @@ -116,6 +128,10 @@ public struct ExpandableHeaderView: View { /// (PM-35398). The caller is responsible for providing a `Binding` whose storage /// outlives the view. /// + /// - Important: A given call site should commit to one initializer variant. Switching + /// between the binding and no-binding initializer at runtime is undefined; see the + /// companion discussion on ``init(title:count:buttonAccessibilityIdentifier:content:)``. + /// /// - Parameters: /// - title: The title of the button used to expand or collapse the content. /// - count: The count of items on the Content. @@ -144,11 +160,29 @@ public struct ExpandableHeaderView: View { #if DEBUG @available(iOS 17, *) -#Preview { +#Preview("Internal state") { + VStack { + ExpandableHeaderView(title: Localizations.localCodes, count: 3) { + BitwardenTextValueField(value: "Option 1") + BitwardenTextValueField(value: "Option 2") + BitwardenTextValueField(value: "Option 3") + } + } + .padding() + .frame(maxHeight: .infinity, alignment: .top) + .background(SharedAsset.Colors.backgroundPrimary.swiftUIColor) +} + +@available(iOS 17, *) +#Preview("Caller-owned binding") { @Previewable @SwiftUI.State var isExpanded = false VStack { - ExpandableHeaderView(title: Localizations.localCodes, count: 3) { + ExpandableHeaderView( + title: Localizations.localCodes, + count: 3, + isExpanded: $isExpanded, + ) { BitwardenTextValueField(value: "Option 1") BitwardenTextValueField(value: "Option 2") BitwardenTextValueField(value: "Option 3")