ref(feedback): Refactor attaching replay id to feedback event to use hooks (#9588)

billyvg · web-flow · commit f24ea887a842 · 2023-11-20T16:21:05.000-05:00
Add a `beforeSendFeedback` hook that replay listens to, to attach
replayId to the feedback event.
diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts
@@ -27,6 +27,7 @@ import type {
   Transport,
   TransportMakeRequestResponse,
 } from '@sentry/types';
+import type { FeedbackEvent } from '@sentry/types';
 import {
   addItemToEnvelope,
   checkOrSetAlreadyCaught,
@@ -413,6 +414,12 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   /** @inheritdoc */
   public on(hook: 'otelSpanEnd', callback: (otelSpan: unknown, mutableOptions: { drop: boolean }) => void): void;
 
+  /** @inheritdoc */
+  public on(
+    hook: 'beforeSendFeedback',
+    callback: (feedback: FeedbackEvent, options?: { includeReplay: boolean }) => void,
+  ): void;
+
   /** @inheritdoc */
   public on(hook: string, callback: unknown): void {
     if (!this._hooks[hook]) {
@@ -450,6 +457,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   /** @inheritdoc */
   public emit(hook: 'otelSpanEnd', otelSpan: unknown, mutableOptions: { drop: boolean }): void;
 
+  /** @inheritdoc */
+  public emit(hook: 'beforeSendFeedback', feedback: FeedbackEvent, options?: { includeReplay: boolean }): void;
+
   /** @inheritdoc */
   public emit(hook: string, ...rest: unknown[]): void {
     if (this._hooks[hook]) {
diff --git a/packages/feedback/src/sendFeedback.ts b/packages/feedback/src/sendFeedback.ts
@@ -1,5 +1,3 @@
-import type { BrowserClient, Replay } from '@sentry/browser';
-import { getCurrentHub } from '@sentry/core';
 import { getLocationHref } from '@sentry/utils';
 
 import { FEEDBACK_API_SOURCE } from './constants';
@@ -19,27 +17,22 @@ interface SendFeedbackParams {
  */
 export function sendFeedback(
   { name, email, message, source = FEEDBACK_API_SOURCE, url = getLocationHref() }: SendFeedbackParams,
-  { includeReplay = true }: SendFeedbackOptions = {},
+  options: SendFeedbackOptions = {},
 ): ReturnType<typeof sendFeedbackRequest> {
-  const client = getCurrentHub().getClient<BrowserClient>();
-  const replay = includeReplay && client ? (client.getIntegrationById('Replay') as Replay | undefined) : undefined;
-
-  // Prepare session replay
-  replay && replay.flush();
-  const replayId = replay && replay.getReplayId();
-
   if (!message) {
     throw new Error('Unable to submit feedback with empty message');
   }
 
-  return sendFeedbackRequest({
-    feedback: {
-      name,
-      email,
-      message,
-      url,
-      replay_id: replayId,
-      source,
+  return sendFeedbackRequest(
+    {
+      feedback: {
+        name,
+        email,
+        message,
+        url,
+        source,
+      },
     },
-  });
+    options,
+  );
 }
diff --git a/packages/feedback/src/util/prepareFeedbackEvent.ts b/packages/feedback/src/util/prepareFeedbackEvent.ts
@@ -27,6 +27,7 @@ export async function prepareFeedbackEvent({
     scope,
     client,
   )) as FeedbackEvent | null;
+
   if (preparedEvent === null) {
     // Taken from baseclient's `_processEvent` method, where this is handled for errors/transactions
     client.recordDroppedEvent('event_processor', 'feedback', event);
diff --git a/packages/feedback/src/util/sendFeedbackRequest.ts b/packages/feedback/src/util/sendFeedbackRequest.ts
@@ -2,15 +2,16 @@ import { createEventEnvelope, getCurrentHub } from '@sentry/core';
 import type { FeedbackEvent, TransportMakeRequestResponse } from '@sentry/types';
 
 import { FEEDBACK_API_SOURCE, FEEDBACK_WIDGET_SOURCE } from '../constants';
-import type { SendFeedbackData } from '../types';
+import type { SendFeedbackData, SendFeedbackOptions } from '../types';
 import { prepareFeedbackEvent } from './prepareFeedbackEvent';
 
 /**
  * Send feedback using transport
  */
-export async function sendFeedbackRequest({
-  feedback: { message, email, name, source, replay_id, url },
-}: SendFeedbackData): Promise<void | TransportMakeRequestResponse> {
+export async function sendFeedbackRequest(
+  { feedback: { message, email, name, source, url } }: SendFeedbackData,
+  { includeReplay = true }: SendFeedbackOptions = {},
+): Promise<void | TransportMakeRequestResponse> {
   const hub = getCurrentHub();
   const client = hub.getClient();
   const transport = client && client.getTransport();
@@ -26,7 +27,6 @@ export async function sendFeedbackRequest({
         contact_email: email,
         name,
         message,
-        replay_id,
         url,
         source,
       },
@@ -54,6 +54,10 @@ export async function sendFeedbackRequest({
         return;
       }
 
+      if (client && client.emit) {
+        client.emit('beforeSendFeedback', feedbackEvent, { includeReplay: Boolean(includeReplay) });
+      }
+
       const envelope = createEventEnvelope(
         feedbackEvent,
         dsn,
diff --git a/packages/feedback/test/widget/createWidget.test.ts b/packages/feedback/test/widget/createWidget.test.ts
@@ -131,7 +131,7 @@ describe('createWidget', () => {
     });
 
     (sendFeedbackRequest as jest.Mock).mockImplementation(() => {
-      return true;
+      return Promise.resolve(true);
     });
     widget.actor?.el?.dispatchEvent(new Event('click'));
 
@@ -147,16 +147,18 @@ describe('createWidget', () => {
     messageEl.dispatchEvent(new Event('change'));
 
     widget.dialog?.el?.querySelector('form')?.dispatchEvent(new Event('submit'));
-    expect(sendFeedbackRequest).toHaveBeenCalledWith({
-      feedback: {
-        name: 'Jane Doe',
-        email: 'jane@example.com',
-        message: 'My feedback',
-        url: 'http://localhost/',
-        replay_id: undefined,
-        source: 'widget',
+    expect(sendFeedbackRequest).toHaveBeenCalledWith(
+      {
+        feedback: {
+          name: 'Jane Doe',
+          email: 'jane@example.com',
+          message: 'My feedback',
+          url: 'http://localhost/',
+          source: 'widget',
+        },
       },
-    });
+      {},
+    );
 
     // sendFeedbackRequest is async
     await flushPromises();
@@ -214,16 +216,18 @@ describe('createWidget', () => {
     messageEl.value = 'My feedback';
 
     widget.dialog?.el?.querySelector('form')?.dispatchEvent(new Event('submit'));
-    expect(sendFeedbackRequest).toHaveBeenCalledWith({
-      feedback: {
-        name: 'Jane Doe',
-        email: 'jane@example.com',
-        message: 'My feedback',
-        url: 'http://localhost/',
-        replay_id: undefined,
-        source: 'widget',
+    expect(sendFeedbackRequest).toHaveBeenCalledWith(
+      {
+        feedback: {
+          name: 'Jane Doe',
+          email: 'jane@example.com',
+          message: 'My feedback',
+          url: 'http://localhost/',
+          source: 'widget',
+        },
       },
-    });
+      {},
+    );
 
     // sendFeedbackRequest is async
     await flushPromises();
@@ -253,16 +257,18 @@ describe('createWidget', () => {
     messageEl.dispatchEvent(new Event('change'));
 
     widget.dialog?.el?.querySelector('form')?.dispatchEvent(new Event('submit'));
-    expect(sendFeedbackRequest).toHaveBeenCalledWith({
-      feedback: {
-        name: '',
-        email: '',
-        message: 'My feedback',
-        url: 'http://localhost/',
-        replay_id: undefined,
-        source: 'widget',
+    expect(sendFeedbackRequest).toHaveBeenCalledWith(
+      {
+        feedback: {
+          name: '',
+          email: '',
+          message: 'My feedback',
+          url: 'http://localhost/',
+          source: 'widget',
+        },
       },
-    });
+      {},
+    );
 
     // sendFeedbackRequest is async
     await flushPromises();
diff --git a/packages/replay/src/util/addGlobalListeners.ts b/packages/replay/src/util/addGlobalListeners.ts
@@ -57,6 +57,17 @@ export function addGlobalListeners(replay: ReplayContainer): void {
     client.on('finishTransaction', transaction => {
       replay.lastTransaction = transaction;
     });
+
+    // We want to flush replay
+    client.on('beforeSendFeedback', (feedbackEvent, options) => {
+      const replayId = replay.getSessionId();
+      if (options && options.includeReplay && replay.isEnabled() && replayId) {
+        void replay.flush();
+        if (feedbackEvent.contexts && feedbackEvent.contexts.feedback) {
+          feedbackEvent.contexts.feedback.replay_id = replayId;
+        }
+      }
+    });
   }
 }
 
diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts
@@ -6,6 +6,7 @@ import type { DsnComponents } from './dsn';
 import type { DynamicSamplingContext, Envelope } from './envelope';
 import type { Event, EventHint } from './event';
 import type { EventProcessor } from './eventprocessor';
+import type { FeedbackEvent } from './feedback';
 import type { Integration, IntegrationClass } from './integration';
 import type { ClientOptions } from './options';
 import type { Scope } from './scope';
@@ -237,6 +238,16 @@ export interface Client<O extends ClientOptions = ClientOptions> {
    */
   on?(hook: 'otelSpanEnd', callback: (otelSpan: unknown, mutableOptions: { drop: boolean }) => void): void;
 
+  /**
+   * Register a callback when a Feedback event has been prepared.
+   * This should be used to mutate the event. The options argument can hint
+   * about what kind of mutation it expects.
+   */
+  on?(
+    hook: 'beforeSendFeedback',
+    callback: (feedback: FeedbackEvent, options?: { includeReplay?: boolean }) => void,
+  ): void;
+
   /**
    * Fire a hook event for transaction start.
    * Expects to be given a transaction as the second argument.
@@ -291,5 +302,12 @@ export interface Client<O extends ClientOptions = ClientOptions> {
    */
   emit?(hook: 'otelSpanEnd', otelSpan: unknown, mutableOptions: { drop: boolean }): void;
 
+  /**
+   * Fire a hook event for after preparing a feedback event. Events to be given
+   * a feedback event as the second argument, and an optional options object as
+   * third argument.
+   */
+  emit?(hook: 'beforeSendFeedback', feedback: FeedbackEvent, options?: { includeReplay?: boolean }): void;
+
   /* eslint-enable @typescript-eslint/unified-signatures */
 }
