Skip to content
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

Merged
merged 7 commits into from Feb 29, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,107 @@
import { expect } from '@playwright/test';

import { sentryTest } from '../../../../utils/fixtures';
import { envelopeRequestParser, getEnvelopeType } from '../../../../utils/helpers';
import { getCustomRecordingEvents, getReplayEvent, waitForReplayRequest } from '../../../../utils/replayHelpers';

sentryTest(
'should capture feedback (@sentry-internal/feedback import)',
async ({ forceFlushReplay, getLocalTestPath, page }) => {
if (process.env.PW_BUNDLE) {
sentryTest.skip();
}

const reqPromise0 = waitForReplayRequest(page, 0);
const reqPromise1 = waitForReplayRequest(page, 1);
const reqPromise2 = waitForReplayRequest(page, 2);
const feedbackRequestPromise = page.waitForResponse(res => {
const req = res.request();

const postData = req.postData();
if (!postData) {
return false;
}

try {
return getEnvelopeType(req) === 'feedback';
} catch (err) {
return false;
}
});

await page.route('https://dsn.ingest.sentry.io/**/*', route => {
return route.fulfill({
status: 200,
contentType: 'application/json',
body: JSON.stringify({ id: 'test-id' }),
});
});

const url = await getLocalTestPath({ testDir: __dirname });

const [, , replayReq0] = await Promise.all([page.goto(url), page.getByText('Report a Bug').click(), reqPromise0]);

// Inputs are slow, these need to be serial
await page.locator('[name="name"]').fill('Jane Doe');
await page.locator('[name="email"]').fill('janedoe@example.org');
await page.locator('[name="message"]').fill('my example feedback');

// Force flush here, as inputs are slow and can cause click event to be in unpredictable segments
await Promise.all([forceFlushReplay(), reqPromise1]);

const [, feedbackResp, replayReq2] = await Promise.all([
page.getByLabel('Send Bug Report').click(),
feedbackRequestPromise,
reqPromise2,
]);

const feedbackEvent = envelopeRequestParser(feedbackResp.request());
const replayEvent = getReplayEvent(replayReq0);
// Feedback breadcrumb is on second segment because we flush when "Report a Bug" is clicked
// And then the breadcrumb is sent when feedback form is submitted
const { breadcrumbs } = getCustomRecordingEvents(replayReq2);

expect(breadcrumbs).toEqual(
expect.arrayContaining([
expect.objectContaining({
category: 'sentry.feedback',
data: { feedbackId: expect.any(String) },
timestamp: expect.any(Number),
type: 'default',
}),
]),
);

expect(feedbackEvent).toEqual({
type: 'feedback',
breadcrumbs: expect.any(Array),
contexts: {
feedback: {
contact_email: 'janedoe@example.org',
message: 'my example feedback',
name: 'Jane Doe',
replay_id: replayEvent.event_id,
source: 'widget',
url: expect.stringContaining('/dist/index.html'),
},
},
level: 'info',
timestamp: expect.any(Number),
event_id: expect.stringMatching(/\w{32}/),
environment: 'production',
sdk: {
integrations: expect.arrayContaining(['Feedback']),
version: expect.any(String),
name: 'sentry.javascript.browser',
packages: expect.anything(),
},
request: {
url: expect.stringContaining('/dist/index.html'),
headers: {
'User-Agent': expect.stringContaining(''),
},
},
platform: 'javascript',
});
},
);
@@ -0,0 +1,18 @@
import { Feedback } from '@sentry-internal/feedback';
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
replaysOnErrorSampleRate: 0,
replaysSessionSampleRate: 0,
integrations: [
new Sentry.Replay({
flushMinDelay: 200,
flushMaxDelay: 200,
minReplayDuration: 0,
}),
new Feedback(),
],
});
@@ -0,0 +1,30 @@
import { expect } from '@playwright/test';

import { sentryTest } from '../../../../utils/fixtures';
import { getReplayEvent, waitForReplayRequest } from '../../../../utils/replayHelpers';

sentryTest(
'should capture feedback with no replay sampling when Form opens (@sentry-internal/feedback import)',
async ({ getLocalTestPath, page }) => {
if (process.env.PW_BUNDLE) {
sentryTest.skip();
}

const reqPromise0 = waitForReplayRequest(page, 0);
await page.route('https://dsn.ingest.sentry.io/**/*', route => {
return route.fulfill({
status: 200,
contentType: 'application/json',
body: JSON.stringify({ id: 'test-id' }),
});
});

const url = await getLocalTestPath({ testDir: __dirname });

const [, , replayReq] = await Promise.all([page.goto(url), page.getByText('Report a Bug').click(), reqPromise0]);

const replayEvent = getReplayEvent(replayReq);
expect(replayEvent.segment_id).toBe(0);
expect(replayEvent.replay_type).toBe('buffer');
},
);

This file was deleted.

3 changes: 3 additions & 0 deletions packages/feedback/package.json
Expand Up @@ -33,6 +33,9 @@
"@sentry/types": "7.100.0",
"@sentry/utils": "7.100.0"
},
"devDependencies": {
"@sentry/replay": "7.100.0"
},
"scripts": {
"build": "run-p build:transpile build:types build:bundle",
"build:transpile": "rollup -c rollup.npm.config.mjs",
Expand Down
38 changes: 32 additions & 6 deletions packages/feedback/src/integration.ts
@@ -1,4 +1,5 @@
import type { Integration, IntegrationFn } from '@sentry/types';
import { type replayIntegration } from '@sentry/replay';
import type { BaseTransportOptions, Client, ClientOptions, Integration, IntegrationFn } from '@sentry/types';
Copy link
Member

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)?

Copy link
Member Author

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

Copy link
Member

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:

  1. 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 🤔
  2. 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?

import { isBrowser, logger } from '@sentry/utils';

import {
Expand Down Expand Up @@ -79,17 +80,18 @@ export class Feedback implements Integration {
private _hasInsertedActorStyles: boolean;

public constructor({
autoInject = true,
id = 'sentry-feedback',
includeReplay = true,
isEmailRequired = false,
isNameRequired = false,
showBranding = true,
autoInject = true,
showEmail = true,
showName = true,
useSentryUser = {
email: 'email',
name: 'username',
},
isEmailRequired = false,
isNameRequired = false,

themeDark,
themeLight,
Expand Down Expand Up @@ -123,9 +125,10 @@ export class Feedback implements Integration {
this._hasInsertedActorStyles = false;

this.options = {
id,
showBranding,
autoInject,
showBranding,
id,
includeReplay,
isEmailRequired,
isNameRequired,
showEmail,
Expand Down Expand Up @@ -185,6 +188,29 @@ export class Feedback implements Integration {
}
}

/**
* Hook that is called after all integrations are setup. This is used to
* integrate with the Replay integration to save a replay when Feedback modal
* is opened.
*/
billyvg marked this conversation as resolved.
Show resolved Hide resolved
public afterAllSetup(client: Client<ClientOptions<BaseTransportOptions>>): void {
if (!this.options.includeReplay) {
return;
}

const replay = client.getIntegrationByName<ReturnType<typeof replayIntegration>>('Replay');

if (!replay) {
return;
}

try {
replay.startBuffering();
} catch (err) {
DEBUG_BUILD && logger.error(err);
}
}

/**
* Allows user to open the dialog box. Creates a new widget if
* `autoInject` was false, otherwise re-uses the default widget that was
Expand Down
5 changes: 5 additions & 0 deletions packages/feedback/src/types/index.ts
Expand Up @@ -72,6 +72,11 @@ export interface FeedbackGeneralConfiguration {
*/
showName: boolean;

/**
* Should Feedback attempt to include a Session Replay if the integration is installed?
*/
includeReplay: boolean;

/**
* Fill in email/name input fields with Sentry user context if it exists.
* The value of the email/name keys represent the properties of your user context.
Expand Down