Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.

feat: Remove breadcrumb from "visibilitychange" event#102

Merged
billyvg merged 4 commits intomainfrom
feat/dedupe-visible-and-focus-crumbs
Jul 6, 2022
Merged

feat: Remove breadcrumb from "visibilitychange" event#102
billyvg merged 4 commits intomainfrom
feat/dedupe-visible-and-focus-crumbs

Conversation

@billyvg
Copy link
Copy Markdown
Member

@billyvg billyvg commented Jun 30, 2022

We're going to axe the "visible" breadcrumb, as it is a point of confusion for users (i.e. what is the difference between blur/focus and visibility?) We will be able to get the information we need with focus/blur alone.

billyvg added 2 commits June 30, 2022 13:52
We handled the blur case but not the focus case in #94
@billyvg billyvg marked this pull request as ready for review June 30, 2022 18:56
@billyvg billyvg requested a review from ryan953 June 30, 2022 18:56
Copy link
Copy Markdown
Member

@ryan953 ryan953 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a race condition or anything between the visibilitychanged event and onfocus? Like, if a page is minimized, it'll be both hidden and blurred. Then when it comes back to the desktop which event will fire first (and turned into a breadcrumb), and which gets dropped?

@billyvg
Copy link
Copy Markdown
Member Author

billyvg commented Jun 30, 2022

There is (and on blur too) -- from what I can tell from the interwebs, the event order is not guaranteed.

@ryan953
Copy link
Copy Markdown
Member

ryan953 commented Jun 30, 2022

If there's no order guarantee then we could get pairs like [blur, visible] or [hidden, focused] which would be weird. I'd expect to see pairs like [blur 0:00, focus 1:00] and [hidden 0:00, visible 1:00]. It would be understandable if those pairs were intermixed on the screen a little, because the timestamps would be about the same at the start & end I could sort it out.

So what if we unconditionally created teh breadcrumb, but then guarded the loadSession, snapshot, flush calls so they are de-duplicated?

@billyvg
Copy link
Copy Markdown
Member Author

billyvg commented Jul 5, 2022

We're going to axe the "visible" breadcrumb, as it is a point of confusion for users (i.e. what is the difference between blur/focus and visibility?) We will be able to get the information we need with focus/blur alone.

@billyvg billyvg changed the title feat: Dedupe visible and focus crumbs feat: Remove breadcrumb from "visibilitychange" event Jul 6, 2022
@billyvg billyvg requested a review from ryan953 July 6, 2022 09:50
@billyvg billyvg merged commit a13dc0a into main Jul 6, 2022
@billyvg billyvg deleted the feat/dedupe-visible-and-focus-crumbs branch July 6, 2022 15:51
mydea pushed a commit to getsentry/sentry-javascript that referenced this pull request Nov 23, 2022
…ry-replay#102)

We're going to axe the "visible" breadcrumb, as it is a point of confusion for users (i.e. what is the difference between blur/focus and visibility?) We will be able to get the information we need with focus/blur alone.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants