-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(feedback): Flush replays when feedback form opens #10567
Conversation
size-limit report 📦
|
3f23a50
to
4069cf5
Compare
3094032
to
bc31cf3
Compare
…form open * By default (can be disabled), if replay integration exists, start buffering * Flush replay when the feedback form is first opened instead of at submit time We are making this change because we have noticed a lot of feedback replays only consist of the user submitting the feedback and not what they did prior to submitting feedback. This may result in false positives if users open but do not submit feedback, but this should make replays from feedback more useful.
bc31cf3
to
c45d03f
Compare
packages/feedback/src/integration.ts
Outdated
@@ -1,4 +1,5 @@ | |||
import type { Integration, IntegrationFn } from '@sentry/types'; | |||
import { type replayIntegration } from '@sentry/replay'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not use this syntax, but import type {}
- I've read that this is better/easier to treeshake!
Also, generally, I'd say maybe we should avoid this - we have this as devDependency only for the type here, which is not really correct from a bundling perspective 😬 I'd rather we just inline the relevant type here (or if really needed put some interface type in @sentry/types
that both packages can use)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't inlining defeat the purpose of typechecking against replay interface? (in this case, it probably wouldn't matter too much, but just trying to think of a good general solution).
I guess the best solution here is to extract it into types
, but it's still a bit annoying having to duplicate the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that's always the problem with the types stuff and the circular dependency stuff 😬
So I'd go with one of these approaches - no strong feelings:
- We add a minimal interface to types - IMHO that may be OK to do for the integration itself, as the surface is quite small and can be considered the public contract. then in replay we can do
class Replay implements ReplayIntegrationInterface
or something like this 🤔 - We inline the styles, which is obv. not super secure but probably OK/fine here (as tests would fail anyhow hopefully if that would change), and we only use a single method right now?
Flush replay when the feedback form is first opened instead of at submit time We are making this change because we have noticed a lot of feedback replays only consist of the user submitting the feedback and not what they did prior to submitting feedback. This may result in false positives if users open but do not submit feedback, but this should make replays from feedback more useful.
Flush replay when the feedback form is first opened instead of at submit time We are making this change because we have noticed a lot of feedback replays only consist of the user submitting the feedback and not what they did prior to submitting feedback. This may result in false positives if users open but do not submit feedback, but this should make replays from feedback more useful.
Have been digging around the code and it looks like this feature has disappeared in v8, despite it still being documented https://docs.sentry.io/platforms/javascript/guides/react/user-feedback/#session-replay |
@philcaonz It's here: sentry-javascript/packages/replay-internal/src/coreHandlers/handleGlobalEvent.ts Lines 40 to 48 in f61e729
|
Flush replay when the feedback form is first opened instead of at submit time
We are making this change because we have noticed a lot of feedback replays only consist of the user submitting the feedback and not what they did prior to submitting feedback. This may result in false positives if users open but do not submit feedback, but this should make replays from feedback more useful.