[PM-26063] Move FlightRecorder into BitwardenKit #2133
Conversation
|
Claude finished @matt-livefront's task —— View job Code Review CompleteThis PR successfully moves the FlightRecorder functionality from BitwardenShared into BitwardenKit, enabling it to be shared with the Authenticator app. The architectural changes are sound and follow established patterns. Summary of Changes Since Last ReviewThe author has addressed previous review feedback:
Key Architectural ChangesModule structure (click to expand)
Moved components (click to expand)Core layer (BitwardenShared → BitwardenKit):
UI layer (BitwardenShared → BitwardenKit):
Resources:
FindingsFinding 1: URL.setIsExcludedFromBackup may not work as intended BitwardenKit/Core/Platform/Extensions/URL.swift:64 The method creates a mutable copy ( func setIsExcludedFromBackup(_ value: Bool) throws {
// Note: Creates mutable copy to call setResourceValues, which modifies
// the file resource attributes at this path, not the URL value itself
var url = self
var values = URLResourceValues()
values.isExcludedFromBackup = value
try url.setResourceValues(values)
}Note: This existed in the original BitwardenShared code. The implementation appears correct based on Apple's documentation, but the comment would improve maintainability. Finding 2: Test coverage gaps in AppModule integration BitwardenShared/UI/Platform/Application/AppModule.swift:66-78 The new Suggested test: /// `makeFlightRecorderCoordinator(stackNavigator:)` returns a properly configured coordinator.
@MainActor
func test_makeFlightRecorderCoordinator() {
let stackNavigator = MockStackNavigator()
let coordinator = subject.makeFlightRecorderCoordinator(stackNavigator: stackNavigator)
XCTAssertNotNil(coordinator)
}Finding 3: Missing test coverage in URL extension BitwardenKit/Core/Platform/Extensions/URL.swift:64-69 The
Good Practices Observed
Action Items
All action items are improvements rather than blocking issues. The PR is architecturally sound and ready for merge pending team review of findings. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
BitwardenKit/Core/Platform/Extensions/FileManager+Extensions.swift
Outdated
Show resolved
Hide resolved
| /// | ||
| /// - Parameter value: `true` if the file should be excluded from backups, or `false` otherwise. | ||
| /// | ||
| func setIsExcludedFromBackup(_ value: Bool) throws { |
There was a problem hiding this comment.
This method creates a mutable copy of self and modifies that copy:
var url = self // Creates a copyThe method name setIsExcludedFromBackup suggests mutation of self, but it's actually modifying a local copy. This should still work because setResourceValues affects the file resource at the URL path, not just the URL value itself.
Recommendation: Add a comment clarifying this behavior:
/// Sets whether the file should be excluded from backups.
///
/// Note: This method creates a mutable copy to call setResourceValues, which
/// modifies the file resource attributes, not the URL value itself.
///
/// - Parameter value: `true` if the file should be excluded from backups, or `false` otherwise.
///
func setIsExcludedFromBackup(_ value: Bool) throws {Note: This pattern existed in the original code. Worth verifying it works as intended, though it's likely correct since URLResourceValues affects file metadata.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2133 +/- ##
==========================================
- Coverage 85.23% 83.77% -1.46%
==========================================
Files 1709 1970 +261
Lines 145427 160762 +15335
==========================================
+ Hits 123958 134686 +10728
- Misses 21469 26076 +4607 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fedemkr
left a comment
fedemkr
left a comment
There was a problem hiding this comment.
👍 Looks good, just a small question
| /// - Returns: The active account's unique identifier. | ||
| /// - Throws: An error if the active account cannot be determined. | ||
| /// | ||
| func getActiveAccountId() async throws -> String |
There was a problem hiding this comment.
🤔 Should the new protocol ActiveAccountStateProvider be used instead of adding the same method here?
There was a problem hiding this comment.
Oh, yeah. Are you thinking we'd have FlightRecorderStateService inherit from ActiveAccountStateProvider?
protocol FlightRecorderStateService: ActiveAccountStateProvider { ... }
There was a problem hiding this comment.
Yes I think that would be nice.
|
Great job! No new security vulnerabilities introduced in this pull request |
🎟️ Tracking
PM-26063
📔 Objective
Moves the flight recorder data, service, and UI into BitwardenKit so this can be shared with Authenticator.
EnableFlightRecorderProcessorandFlightRecorderLogsProcessorpreviously usedSettingsRouteto communicate navigation with their coordinators. As part of this move, I created a newFlightRecorderCoordinatorwithFlightRecorderRoutes to handle navigation between the flight recorder views and make this more of a component that can be shared with Authenticator.⏰ 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