Add more info to crash reports #1253
Conversation
bgrgicak
added
[Type] Enhancement
|
@adamziel I have another PR that refactors the logger and is based on this PR. |
|
Almost there @bgrgicak! Let's also document the info and the service worker change in the PR description so our future selves will be able to very quickly understand this change without diving into the code. |
Co-authored-by: Adam Zielinski <adam@adamziel.com>
Co-authored-by: Adam Zielinski <adam@adamziel.com>
Co-authored-by: Adam Zielinski <adam@adamziel.com>
|
All feedback should now be addressed. I also updated #1264 with these changes. |
| }; | ||
| worker.clients | ||
| .matchAll({ | ||
| includeUncontrolled: true, |
There was a problem hiding this comment.
Why includeUncontrolled? Let's leave a comment in the code to explain. Otherwise LGTM 👍
There was a problem hiding this comment.
@bgrgicak yeah I meant more not the literal meaning of that option but rather why bother with uncontrolled clients.
There was a problem hiding this comment.
I can update it in #1264 with something like this.
// Inform all Playground clients that there was a change in the number of active tabs
There was a problem hiding this comment.
Right, but why an uncontrolled client is considered a Playground client? It seems like that browser tab wouldn't be able to access Playground anyway, as being controlled is crucial for that, right?
There was a problem hiding this comment.
Correct, I tested this a bit and it's actually not needed, so I will remove this line in #1264
Fixes #1152
What is this PR doing?
It adds environment context data and the blueprint to crash reports.
What problem is it solving?
This should help us debug crash reports.
How is the problem addressed?
By adding more data to the crash report.
Testing Instructions