fix: Page titles in breadcrumbs should not change #551
Conversation
…reference, preventing breadcrumb integration from modifying renderer state
| @@ -183,14 +183,10 @@ export class ElectronBreadcrumbs implements Integration { | |||
| }; | |||
|
|
|||
| if (id) { | |||
| const state = getRendererProperties(id); | |||
| breadcrumb.data = { ...getRendererProperties(id) }; | |||
There was a problem hiding this comment.
getRendererProperties could return null
There was a problem hiding this comment.
Unless I'm mistaken, getRendererProperties should only ever return some object or undefined, which this change should handle. Am I overlooking something?
There was a problem hiding this comment.
Oh interesting, I didn't think ...null was valid JS 🤔 Apparently it is?
There was a problem hiding this comment.
Yeah this should be fine to cover the case here.
There was a problem hiding this comment.
Thanks for the patch! Took the liberty of editing the title to match our conventions. We can try to get this released tomorrow, @timfish does that work for you?
AbhiPrasad
changed the title
AbhiPrasad
changed the title
timfish
changed the title
|
@timfish - We've got some folks asking if we could write a test for this fix, to make sure there's never a regression in the future. Is that something you might be able to help with? |
|
Yes, I will add some tests! |
|
Thank you!!! |
|
Thanks, @timfish! |
We noticed an issue where page titles were showing up in breadcrumbs and discovered that the breadcrumbs'
dataproperties are references toRENDERERSobject here. Thus, if the page title changes after a breadcrumb is created but before it is sent with an event, this handler will also change thedataproperty of the breadcrumb.This change prevents that by setting the
dataproperty of the breadcrumb to a clone of the renderer state. Also, this change prevents the breadcrumb integration from modifying the renderer state when writing itsdataprop.