[PM-19621] Add share and delete options for flight recorder logs #1519
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1519 +/- ##
==========================================
+ Coverage 89.67% 89.71% +0.04%
==========================================
Files 779 779
Lines 49117 49239 +122
==========================================
+ Hits 44044 44176 +132
+ Misses 5073 5063 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| await stateService.setFlightRecorderData(data) | ||
| } | ||
|
|
||
| func deleteLog(_ log: FlightRecorderLogMetadata) async throws { |
There was a problem hiding this comment.
🤔 Would it make sense to have deleteArchivedLogs just call this function instead of doing the delete itself? Or is it just not worth doing the guard checks?
There was a problem hiding this comment.
Good question. I think in that case, we would need to convert the log to a FlightRecorderLogMetadata and it would result in writing out the updated FlightRecorderData to user defaults for each log that's being deleted. I did refactor the do...catch block that does the actual delete into a separate method in some more recent changes though which ends up removing most of the duplication between them.
There was a problem hiding this comment.
Ah, I missed that they were different enough objects. Makes sense, then.
| private func deleteAllLogs() { | ||
| confirmDeletion(isBulkDeletion: true) { | ||
| do { | ||
| try await self.services.flightRecorder.deleteArchivedLogs() |
There was a problem hiding this comment.
🤔 Is there a reason for the verbiage shift from "all logs" to "archived logs", or is this just a difference in how it's presented to the user vs. how we organize it internally?
There was a problem hiding this comment.
a difference in how it's presented to the user vs. how we organize it internally?
Yeah, the UI refers to it as "all logs", but I was trying to be more precise in the services definition to make it clear that it only deletes any archived logs and not the active log. This seemed like the best place to draw that line, but I do see the potential confusion 🤔.
There was a problem hiding this comment.
Maybe deleteNonActiveLogs or deleteInactiveLogs to be a bit more clear? 🤔 the distinction absolutely makes sense, though—I just was expecting "archive" to be something a user could do to a log as well. I think it's ultimately fine either way, just confused me for a bit.
There was a problem hiding this comment.
Ah, I'll rename it to inactive, I think that'll be more clear.
4bd8e78 to
aec2a18
Compare
🎟️ Tracking
PM-19621
📔 Objective
Adds the share and deletion option for flight recorder logs.
📸 Screenshots
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-04-22.at.16.26.20.mp4
⏰ 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