[PM-18223] Add share error details button to error alerts without a message #1438
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1438 +/- ##
==========================================
- Coverage 89.58% 85.78% -3.81%
==========================================
Files 767 984 +217
Lines 48194 58509 +10315
==========================================
+ Hits 43176 50191 +7015
- Misses 5018 8318 +3300 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job, no security vulnerabilities found in this Pull Request |
There was a problem hiding this comment.
🤔 Even though this approach makes sense, I'm not very fond of passing the services through a function to the Coordinator, although I know it's tricky given that it's a protocol.
Do we only need the services for the feature flag check? Or are you planning to use more services in the Coordinator -> showErrorAlert function? Or well anywhere in the Coordinator?
🤔 I was thinking about having some sort of proxy or composed/wrapped Coordinator so you can add any additional logic without disrupting the original implementation. Therefore you can pass any coordinator to be wrapped in the composed one to add this feature flag logic change.
| // MARK: Types | ||
|
|
||
| typealias ErrorAlertServices = HasConfigService & HasErrorReporter | ||
| typealias ErrorAlertTryAgain = (() async -> Void)? |
There was a problem hiding this comment.
🤔 Why is this typealias needed? Couldn't the closure be used in-situ or you wanted to be flexible to be changed in all places in the future just by changing it here?
There was a problem hiding this comment.
Not strictly necessary, but it was used in enough places where it seemed nice to have. At one point it wasn't async and I made it async and that triggered a trickle down of errors so I liked the convenience of just defining it once. I'm not particularly tied to it if you prefer to define it in place though.
| // MARK: Types | ||
|
|
||
| typealias ErrorAlertServices = HasConfigService & HasErrorReporter |
There was a problem hiding this comment.
🤔 Is errorReporter service going to be used in a future PR? Because I'm not seeing that used here.
There was a problem hiding this comment.
My plan was to put the logic of building the error details to share within that service. So once you tap 'Share error details' it would assemble the error details and app info which could then be shared.
We need the services for the feature flag check and possibly some other service that builds the error details + app info that is ultimately shared. We also have an For the flight recording feature, we talked about possibly logging breadcrumbs. The easiest place to put this could be in the coordinator, or a coordinator base class? For this reason, I kind of had in my head that we might need to create a coordinator base class (or similar solution) which has access to the coordinator's services. And if we did that we could stop passing the services around here. I have similar concerns around passing services, but putting this logic in the coordinator seemed like the best spot and it feels like we might be headed in a direction that we could get the services directly from the coordinator and avoid this passing.
This kind of sounds like what |
There was a problem hiding this comment.
🤔 I think this is is better than the previous solution @matt-livefront and then as you mentioned before we can explore having a base one but I think this lays the grounds for that.
The only other thing I was thinking is: if a coordinator doesn't conform to the new protocol then it wouldn't display the new alert but it would have to have a custom implementation for showErrorAlert, right? So I believe it's a "safe" way to know we're missing that protocol in a new Coordinator.
What do you think @KatherineInCode ?
| func showErrorAlert(error: any Error, tryAgain: (() async -> Void)?) async { | ||
| errorReporter.log(error: BitwardenError.generalError( | ||
| type: "TabCoordinator: `showErrorAlert` Not Supported", | ||
| message: "`showErrorAlert(error:tryAgain:)` is not supported from TabCoordinator." | ||
| )) | ||
| } |
There was a problem hiding this comment.
🤔 Is this not supported here because the TabCoordinator has only other internal coordinators which would handle errors?
There was a problem hiding this comment.
Correct, it doesn't build any processors that would ultimately call this on this coordinator. It also doesn't have access to the service container currently, which was the main reason I didn't just implement the protocol.
| await coordinator.showErrorAlert(error: error) | ||
| } | ||
| services.errorReporter.log(error: error) |
There was a problem hiding this comment.
🤔 Now that I see this, I think one think to keep in mind would be to almost always log to the error reporter first before showing any error alert; so in here services.errorReporter.log(error: error) should be at the beginning of the catch instead of at the end. Therefore if the user/OS kills the app while the alert is being shown the error gets reported nonetheless to Crashlytics.
If I'm thinking this correctly if the above happens now we're going to miss the error report because the catch clause will await for the error alert to finish in order to log in the ErrorReporter.
There was a problem hiding this comment.
I can move up the error reporter call. Even though showErrorAlert is async though, it will continue on after the coordinator calls present on the alert controller as we don't continue waiting for the alert to be dismissed. So probably minimal risk to the app being killed before the log call.
|
Also @matt-livefront sorry I didn't answer the previous comment earlier, thanks so much for the additional detailed information 😄 It has been really helpful to understand everything. |
@fedemkr Exactly, you'll get a build error that the coordinator doesn't conform to the |
|
@fedemkr @matt-livefront I do think exploring what a base Coordinator might look like may actually be the best route to go. I'm a little concerned about us making Coordinator things internal as well, because ultimately we'll want to move the |
I removed some public access levels on I think it may still be worth exploring a base class, especially if we want breadcrumb logging from the coordinator. But maybe as a separate effort as this approach seemed less invasive. On one hand, the approach I landed on might be a little more flexible depending on where you draw the line for what goes in |
Which is something we're still hashing out a bit, and will be able to a bit more once I get back to finishing up the first wave of monorepo work. To a certain extent, I think we might want to pull a lot of those services into I do think exploring a base class might be a good idea. I'm usually about composition over inheritance, but this seems like one of the few places doing inheritance might be better. |
Yep, that all makes sense. |
|
In my opinion when we move these classes to the |
|
I believe we can approve as is or would you like any other changes @KatherineInCode done directly in this PR to prepare for the broader refactor/preparation for the Kit? |
|
I agree that the move to I have no objections to moving forward as-is. |
🎟️ Tracking
PM-18381 - Add Feature Flag for Error Reporting
PM-18223 - Implement 'Share error details' Button in Error Dialogs
📔 Objective
This adds the mobile error reporting feature flag and implements the foundational pieces to support showing the 'Share error details' button in error alerts.
The 'Share error details' button will show for any default alerts which don't already show a message. Because 'Share error details' will need access to some services and the coordinator to show a share sheet, I've changed how displaying default error alerts works.
Before:
coordinator.showAlert(.networkResponseError(error))After:
await coordinator.showErrorAlert(error: error, services: services)This was the best compromise to keep this to a one-liner in a processor when presenting this alert, while supporting this new functionality. I tried writing an extension on
Coordinatorthat would allow access toserviceswithout needing to pass those in, but ultimately couldn't get it to work with the protocol definition that we have. I think we would instead need aCoordinatorbase class, which we could explore in the future if needed.To keep this PR small and focused on the setup pieces, I only updated
VaultListProcessorto use this new pattern and will follow up with another PR updating the rest of the app.Note
This introduces a few SwiftLint warnings, which will be resolved in the next PR:
📸 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