[PM-26063] Add reusable component for the Flight Recorder settings view #2141
Conversation
|
Claude finished @matt-livefront's task —— View job Code Review CompleteSummaryThis PR successfully refactors Flight Recorder settings into a reusable component in BitwardenKit. The implementation follows the established unidirectional data flow architecture and integrates cleanly with the existing AboutView. The changes reduce code duplication and enable easier sharing with the Authenticator app. Previous review findings: The 2 access modifier findings from the previous review were not addressed in these commits. Both remain valid concerns (see Findings 2 and 3 below). Critical IssuesFinding 1: Snapshot tests are disabled and will not run in CI The snapshot tests have // FlightRecorderSettingsSectionView+SnapshotTests.swift:54
func disabletest_snapshot_flightRecorderSettingsSection_disabled() {
// FlightRecorderSettingsSectionView+SnapshotTests.swift:63
func disabletest_snapshot_flightRecorderSettingsSection_enabled() {This defeats the purpose of snapshot testing for catching visual regressions. The tests should be enabled. Minor IssuesFinding 2: Store property should be marked as BitwardenKit/UI/Platform/Settings/FlightRecorder/FlightRecorderSettingsSection/FlightRecorderSettingsSectionView.swift:30 The Comparison with similar componentsLooking at other reusable views in BitwardenKit like Finding 3: Computed property should be marked as BitwardenKit/UI/Platform/Settings/FlightRecorder/FlightRecorderSettingsSection/FlightRecorderSettingsSectionState.swift:20 The Code Quality ImprovementsFinding 4: Test coverage gap for accessibility label with active log FlightRecorderSettingsSectionStateTests.swift only tests the accessibility label in two states (on/off), but doesn't verify the formatted date/time values are correct in the label. Consider adding a test that validates the exact format matches expectations, especially since there's custom DateFormatter logic for VoiceOver. Finding 5: Missing DocC documentation comment BitwardenKit/UI/Platform/Settings/FlightRecorder/FlightRecorderSettingsSection/FlightRecorderSettingsSectionView.swift:30 Per the project guidelines in Good Practices Observed
Action Items
|
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2141 +/- ##
==========================================
- Coverage 85.23% 83.91% -1.33%
==========================================
Files 1714 1981 +267
Lines 145520 160891 +15371
==========================================
+ Hits 124040 135004 +10964
- Misses 21480 25887 +4407 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @Environment(\.openURL) private var openURL | ||
|
|
||
| /// The `Store` for this view. | ||
| @ObservedObject var store: FlightRecorderSettingsSectionStore |
There was a problem hiding this comment.
🎨 Finding 1: Store property should be marked as public
The store property is internal, but this is a public struct in BitwardenKit meant to be used by other modules (like BitwardenShared). For consistency with other public views in the codebase, this property should be marked public.
| @ObservedObject var store: FlightRecorderSettingsSectionStore | |
| public var store: FlightRecorderSettingsSectionStore |
| // MARK: Computed Properties | ||
|
|
||
| /// The accessibility label for the Flight Recorder toggle. | ||
| var flightRecorderToggleAccessibilityLabel: String { |
There was a problem hiding this comment.
🎨 Finding 2: Computed property should be marked as public
The flightRecorderToggleAccessibilityLabel computed property is internal, but it's accessed from a public view. For consistency and to make the intent clear that this is part of the public API of this state, it should be marked public.
| var flightRecorderToggleAccessibilityLabel: String { | |
| public var flightRecorderToggleAccessibilityLabel: String { |
|
|
||
| /// The flight recorder settings section view renders correctly when disabled. | ||
| @MainActor | ||
| func disabletest_snapshot_flightRecorderSettingsSection_disabled() { |
There was a problem hiding this comment.
❌ Finding 1: Snapshot tests are disabled
The disabletest prefix prevents these tests from running. They should be named test_snapshot_flightRecorderSettingsSection_disabled to execute in CI and catch visual regressions.
| func disabletest_snapshot_flightRecorderSettingsSection_disabled() { | |
| func test_snapshot_flightRecorderSettingsSection_disabled() { |
|
|
||
| /// The flight recorder settings section view renders correctly when enabled. | ||
| @MainActor | ||
| func disabletest_snapshot_flightRecorderSettingsSection_enabled() { |
There was a problem hiding this comment.
❌ Finding 1: Snapshot tests are disabled
The disabletest prefix prevents these tests from running. They should be named test_snapshot_flightRecorderSettingsSection_enabled to execute in CI and catch visual regressions.
| func disabletest_snapshot_flightRecorderSettingsSection_enabled() { | |
| func test_snapshot_flightRecorderSettingsSection_enabled() { |
| @Environment(\.openURL) private var openURL | ||
|
|
||
| /// The `Store` for this view. | ||
| @ObservedObject var store: FlightRecorderSettingsSectionStore |
There was a problem hiding this comment.
public
This property is internal but the struct is public and meant to be used across module boundaries (BitwardenKit → BitwardenShared). Making it public clarifies the API contract.
Per project guidelines, all public properties require DocC documentation. Add a comment explaining the store's purpose.
| @ObservedObject var store: FlightRecorderSettingsSectionStore | |
| /// The `Store` for this view. | |
| @ObservedObject public var store: FlightRecorderSettingsSectionStore |
| // MARK: Computed Properties | ||
|
|
||
| /// The accessibility label for the Flight Recorder toggle. | ||
| var flightRecorderToggleAccessibilityLabel: String { |
There was a problem hiding this comment.
public
This computed property is accessed from a public view and is part of the public API of this state struct. Making it public clarifies this intent and ensures consistent access patterns.
| var flightRecorderToggleAccessibilityLabel: String { | |
| public var flightRecorderToggleAccessibilityLabel: String { |
matt-livefront
deleted the
matt/PM-26063-flight-recorder-settings-section
branch
🎟️ Tracking
PM-26063
📔 Objective
Adds a reusable component to BitwardenKit for the Flight Recorder section within the settings view of the app. This will make it so this component can more easily be shared with the Authenticator.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes