From c682d2b3560a2cc6a7ef8981cac256abee14128c Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Thu, 16 Nov 2023 16:54:29 -0500 Subject: [PATCH 01/18] isNameRequired and isEmailRequired --- packages/feedback/src/widget/Dialog.ts | 4 ++ packages/feedback/src/widget/Form.ts | 40 ++++++++++++++++---- packages/feedback/src/widget/createWidget.ts | 2 + packages/feedback/test/widget/Dialog.test.ts | 4 ++ packages/feedback/test/widget/Form.test.ts | 36 +++++++++++++++++- 5 files changed, 76 insertions(+), 10 deletions(-) diff --git a/packages/feedback/src/widget/Dialog.ts b/packages/feedback/src/widget/Dialog.ts index 8fdfbba72f20..35a4c43b7bce 100644 --- a/packages/feedback/src/widget/Dialog.ts +++ b/packages/feedback/src/widget/Dialog.ts @@ -45,6 +45,8 @@ export function Dialog({ showBranding, showName, showEmail, + isNameRequired, + isEmailRequired, colorScheme, isAnonymous, defaultName, @@ -102,6 +104,8 @@ export function Dialog({ showEmail, showName, isAnonymous, + isEmailRequired, + isNameRequired, defaultName, defaultEmail, diff --git a/packages/feedback/src/widget/Form.ts b/packages/feedback/src/widget/Form.ts index 74ff06c015b9..335e60611a02 100644 --- a/packages/feedback/src/widget/Form.ts +++ b/packages/feedback/src/widget/Form.ts @@ -8,6 +8,8 @@ export interface FormComponentProps | 'showName' | 'showEmail' | 'isAnonymous' + | 'isNameRequired' + | 'isEmailRequired' | Exclude > { /** @@ -58,6 +60,8 @@ export function Form({ showName, showEmail, isAnonymous, + isNameRequired, + isEmailRequired, defaultName, defaultEmail, @@ -110,9 +114,10 @@ export function Form({ const nameEl = createElement('input', { id: 'name', - type: showName ? 'text' : 'hidden', - ['aria-hidden']: showName ? 'false' : 'true', + type: showName || isNameRequired? 'text' : 'hidden', + ['aria-hidden']: showName || isNameRequired? 'false' : 'true', name: 'name', + required: isNameRequired, className: 'form__input', placeholder: namePlaceholder, value: defaultName, @@ -120,9 +125,10 @@ export function Form({ const emailEl = createElement('input', { id: 'email', - type: showEmail ? 'text' : 'hidden', - ['aria-hidden']: showEmail ? 'false' : 'true', + type: showEmail || isEmailRequired? 'text' : 'hidden', + ['aria-hidden']: showEmail || isEmailRequired? 'false' : 'true', name: 'email', + required: isEmailRequired, className: 'form__input', placeholder: emailPlaceholder, value: defaultEmail, @@ -161,28 +167,46 @@ export function Form({ errorEl, !isAnonymous && - showName && + (showName || isNameRequired) && createElement( 'label', { htmlFor: 'name', className: 'form__label', }, + isNameRequired ? [ + createElement( + 'span', + { className: 'form__label__text' }, + nameLabel, + createElement('span', { className: 'form__label__text--required' }, ' (required)'), + ), + nameEl, + ] : [nameLabel, nameEl], ), - !isAnonymous && !showName && nameEl, + !isAnonymous && !(showName||isNameRequired) && nameEl, !isAnonymous && - showEmail && + (showEmail || isEmailRequired) && createElement( 'label', { htmlFor: 'email', className: 'form__label', }, + isEmailRequired ? [ + createElement( + 'span', + { className: 'form__label__text' }, + emailLabel, + createElement('span', { className: 'form__label__text--required' }, ' (required)'), + ), + emailEl, + ] : [emailLabel, emailEl], ), - !isAnonymous && !showEmail && emailEl, + !isAnonymous && !(showEmail||isEmailRequired) && emailEl, createElement( 'label', diff --git a/packages/feedback/src/widget/createWidget.ts b/packages/feedback/src/widget/createWidget.ts index 480e3d476219..d126c35aa4ba 100644 --- a/packages/feedback/src/widget/createWidget.ts +++ b/packages/feedback/src/widget/createWidget.ts @@ -159,6 +159,8 @@ export function createWidget({ showName: options.showName, showEmail: options.showEmail, isAnonymous: options.isAnonymous, + isNameRequired: options.isNameRequired, + isEmailRequired: options.isEmailRequired, formTitle: options.formTitle, cancelButtonLabel: options.cancelButtonLabel, submitButtonLabel: options.submitButtonLabel, diff --git a/packages/feedback/test/widget/Dialog.test.ts b/packages/feedback/test/widget/Dialog.test.ts index d0968e6e6030..e601f4783d96 100644 --- a/packages/feedback/test/widget/Dialog.test.ts +++ b/packages/feedback/test/widget/Dialog.test.ts @@ -10,6 +10,8 @@ function renderDialog({ showEmail = true, showBranding = false, isAnonymous = false, + isNameRequired = false, + isEmailRequired = false, formTitle = 'Feedback', defaultName = 'Foo Bar', defaultEmail = 'foo@example.com', @@ -30,6 +32,8 @@ function renderDialog({ isAnonymous, showName, showEmail, + isNameRequired, + isEmailRequired, showBranding, defaultName, defaultEmail, diff --git a/packages/feedback/test/widget/Form.test.ts b/packages/feedback/test/widget/Form.test.ts index 4eb1c925c007..e7b629ac318a 100644 --- a/packages/feedback/test/widget/Form.test.ts +++ b/packages/feedback/test/widget/Form.test.ts @@ -9,6 +9,8 @@ function renderForm({ showName = true, showEmail = true, isAnonymous = false, + isNameRequired = false, + isEmailRequired = false, defaultName = 'Foo Bar', defaultEmail = 'foo@example.com', nameLabel = 'Name', @@ -25,6 +27,8 @@ function renderForm({ isAnonymous, showName, showEmail, + isNameRequired, + isEmailRequired, defaultName, defaultEmail, nameLabel, @@ -80,13 +84,15 @@ describe('Form', () => { emailPlaceholder: 'foo@example.org!', messageLabel: 'Description!', messagePlaceholder: 'What is the issue?!', + isNameRequired: true, + isEmailRequired: true, }); const nameLabel = formComponent.el.querySelector('label[htmlFor="name"]') as HTMLLabelElement; const emailLabel = formComponent.el.querySelector('label[htmlFor="email"]') as HTMLLabelElement; const messageLabel = formComponent.el.querySelector('label[htmlFor="message"]') as HTMLLabelElement; - expect(nameLabel.textContent).toBe('Name!'); - expect(emailLabel.textContent).toBe('Email!'); + expect(nameLabel.textContent).toBe('Name! (required)'); + expect(emailLabel.textContent).toBe('Email! (required)'); expect(messageLabel.textContent).toBe('Description! (required)'); const nameInput = formComponent.el.querySelector('[name="name"]') as HTMLInputElement; @@ -110,6 +116,30 @@ describe('Form', () => { message.dispatchEvent(new KeyboardEvent('keyup')); }); + it('submit is enabled if name is required and is not empty', () => { + const formComponent = renderForm({isNameRequired: true}); + + const name = formComponent.el.querySelector('[name="name"]') as HTMLTextAreaElement; + + name.value = 'Foo Bar'; + name.dispatchEvent(new KeyboardEvent('keyup')); + + name.value = ''; + name.dispatchEvent(new KeyboardEvent('keyup')); + }); + + it('submit is enabled if email is required and is not empty', () => { + const formComponent = renderForm({isEmailRequired: true}); + + const email = formComponent.el.querySelector('[name="email"]') as HTMLTextAreaElement; + + email.value = 'foo@example.org'; + email.dispatchEvent(new KeyboardEvent('keyup')); + + email.value = ''; + email.dispatchEvent(new KeyboardEvent('keyup')); + }); + it('can show error', () => { const formComponent = renderForm(); const errorEl = formComponent.el.querySelector('.form__error-container') as HTMLDivElement; @@ -148,6 +178,8 @@ describe('Form', () => { it('does not show name or email inputs for anonymous mode', () => { const onSubmit = jest.fn(); const formComponent = renderForm({ + isNameRequired: true, + isEmailRequired: true, isAnonymous: true, onSubmit, }); From 9086e25eaa438d79c72f5d48c7d51652a45732ec Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Thu, 16 Nov 2023 17:28:58 -0500 Subject: [PATCH 02/18] lint --- packages/feedback/src/widget/Form.ts | 54 +++++++++++----------- packages/feedback/test/widget/Form.test.ts | 4 +- 2 files changed, 30 insertions(+), 28 deletions(-) diff --git a/packages/feedback/src/widget/Form.ts b/packages/feedback/src/widget/Form.ts index 335e60611a02..f0f35726d259 100644 --- a/packages/feedback/src/widget/Form.ts +++ b/packages/feedback/src/widget/Form.ts @@ -114,8 +114,8 @@ export function Form({ const nameEl = createElement('input', { id: 'name', - type: showName || isNameRequired? 'text' : 'hidden', - ['aria-hidden']: showName || isNameRequired? 'false' : 'true', + type: showName || isNameRequired ? 'text' : 'hidden', + ['aria-hidden']: showName || isNameRequired ? 'false' : 'true', name: 'name', required: isNameRequired, className: 'form__input', @@ -125,8 +125,8 @@ export function Form({ const emailEl = createElement('input', { id: 'email', - type: showEmail || isEmailRequired? 'text' : 'hidden', - ['aria-hidden']: showEmail || isEmailRequired? 'false' : 'true', + type: showEmail || isEmailRequired ? 'text' : 'hidden', + ['aria-hidden']: showEmail || isEmailRequired ? 'false' : 'true', name: 'email', required: isEmailRequired, className: 'form__input', @@ -174,18 +174,19 @@ export function Form({ htmlFor: 'name', className: 'form__label', }, - isNameRequired ? [ - createElement( - 'span', - { className: 'form__label__text' }, - nameLabel, - createElement('span', { className: 'form__label__text--required' }, ' (required)'), - ), - nameEl, - ] : - [nameLabel, nameEl], + isNameRequired + ? [ + createElement( + 'span', + { className: 'form__label__text' }, + nameLabel, + createElement('span', { className: 'form__label__text--required' }, ' (required)'), + ), + nameEl, + ] + : [nameLabel, nameEl], ), - !isAnonymous && !(showName||isNameRequired) && nameEl, + !isAnonymous && !(showName || isNameRequired) && nameEl, !isAnonymous && (showEmail || isEmailRequired) && @@ -195,18 +196,19 @@ export function Form({ htmlFor: 'email', className: 'form__label', }, - isEmailRequired ? [ - createElement( - 'span', - { className: 'form__label__text' }, - emailLabel, - createElement('span', { className: 'form__label__text--required' }, ' (required)'), - ), - emailEl, - ] : - [emailLabel, emailEl], + isEmailRequired + ? [ + createElement( + 'span', + { className: 'form__label__text' }, + emailLabel, + createElement('span', { className: 'form__label__text--required' }, ' (required)'), + ), + emailEl, + ] + : [emailLabel, emailEl], ), - !isAnonymous && !(showEmail||isEmailRequired) && emailEl, + !isAnonymous && !(showEmail || isEmailRequired) && emailEl, createElement( 'label', diff --git a/packages/feedback/test/widget/Form.test.ts b/packages/feedback/test/widget/Form.test.ts index e7b629ac318a..efea51f03ab8 100644 --- a/packages/feedback/test/widget/Form.test.ts +++ b/packages/feedback/test/widget/Form.test.ts @@ -117,7 +117,7 @@ describe('Form', () => { }); it('submit is enabled if name is required and is not empty', () => { - const formComponent = renderForm({isNameRequired: true}); + const formComponent = renderForm({ isNameRequired: true }); const name = formComponent.el.querySelector('[name="name"]') as HTMLTextAreaElement; @@ -129,7 +129,7 @@ describe('Form', () => { }); it('submit is enabled if email is required and is not empty', () => { - const formComponent = renderForm({isEmailRequired: true}); + const formComponent = renderForm({ isEmailRequired: true }); const email = formComponent.el.querySelector('[name="email"]') as HTMLTextAreaElement; From f185df64b9a2aa0df83883ae796f8f129db5f28f Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Fri, 17 Nov 2023 15:25:31 -0500 Subject: [PATCH 03/18] comments --- packages/feedback/src/widget/Form.ts | 56 ++++++++-------- packages/feedback/src/widget/createWidget.ts | 16 ++++- packages/feedback/test/widget/Form.test.ts | 36 ---------- .../feedback/test/widget/createWidget.test.ts | 66 +++++++++++++++++++ 4 files changed, 106 insertions(+), 68 deletions(-) diff --git a/packages/feedback/src/widget/Form.ts b/packages/feedback/src/widget/Form.ts index f0f35726d259..50fd49fd9528 100644 --- a/packages/feedback/src/widget/Form.ts +++ b/packages/feedback/src/widget/Form.ts @@ -114,8 +114,8 @@ export function Form({ const nameEl = createElement('input', { id: 'name', - type: showName || isNameRequired ? 'text' : 'hidden', - ['aria-hidden']: showName || isNameRequired ? 'false' : 'true', + type: showName ? 'text' : 'hidden', + ['aria-hidden']: showName ? 'false' : 'true', name: 'name', required: isNameRequired, className: 'form__input', @@ -125,8 +125,8 @@ export function Form({ const emailEl = createElement('input', { id: 'email', - type: showEmail || isEmailRequired ? 'text' : 'hidden', - ['aria-hidden']: showEmail || isEmailRequired ? 'false' : 'true', + type: showEmail ? 'text' : 'hidden', + ['aria-hidden']: showEmail ? 'false' : 'true', name: 'email', required: isEmailRequired, className: 'form__input', @@ -167,48 +167,44 @@ export function Form({ errorEl, !isAnonymous && - (showName || isNameRequired) && + showName && createElement( 'label', { htmlFor: 'name', className: 'form__label', }, - isNameRequired - ? [ - createElement( - 'span', - { className: 'form__label__text' }, - nameLabel, - createElement('span', { className: 'form__label__text--required' }, ' (required)'), - ), - nameEl, - ] - : [nameLabel, nameEl], + [ + createElement( + 'span', + { className: 'form__label__text' }, + nameLabel, + isNameRequired && createElement('span', { className: 'form__label__text--required' }, ' (required)'), + ), + nameEl, + ], ), - !isAnonymous && !(showName || isNameRequired) && nameEl, + !isAnonymous && !showName && nameEl, !isAnonymous && - (showEmail || isEmailRequired) && + showEmail && createElement( 'label', { htmlFor: 'email', className: 'form__label', }, - isEmailRequired - ? [ - createElement( - 'span', - { className: 'form__label__text' }, - emailLabel, - createElement('span', { className: 'form__label__text--required' }, ' (required)'), - ), - emailEl, - ] - : [emailLabel, emailEl], + [ + createElement( + 'span', + { className: 'form__label__text' }, + emailLabel, + isEmailRequired && createElement('span', { className: 'form__label__text--required' }, ' (required)'), + ), + emailEl, + ], ), - !isAnonymous && !(showEmail || isEmailRequired) && emailEl, + !isAnonymous && !showEmail && emailEl, createElement( 'label', diff --git a/packages/feedback/src/widget/createWidget.ts b/packages/feedback/src/widget/createWidget.ts index d126c35aa4ba..42ebbd962a02 100644 --- a/packages/feedback/src/widget/createWidget.ts +++ b/packages/feedback/src/widget/createWidget.ts @@ -94,6 +94,18 @@ export function createWidget({ return; } + // Simple validation for now, just check for non-empty message if name required + if (options.isNameRequired && !feedback.name) { + dialog.showError('Please enter in a name before submitting!'); + return; + } + + // Simple validation for now, just check for non-empty message if name required + if (options.isEmailRequired && !feedback.email) { + dialog.showError('Please enter in an email before submitting!'); + return; + } + const result = await handleFeedbackSubmit(dialog, feedback); // Error submitting feedback @@ -156,8 +168,8 @@ export function createWidget({ dialog = Dialog({ colorScheme: options.colorScheme, showBranding: options.showBranding, - showName: options.showName, - showEmail: options.showEmail, + showName: options.showName || options.isNameRequired, + showEmail: options.showEmail || options.isEmailRequired, isAnonymous: options.isAnonymous, isNameRequired: options.isNameRequired, isEmailRequired: options.isEmailRequired, diff --git a/packages/feedback/test/widget/Form.test.ts b/packages/feedback/test/widget/Form.test.ts index efea51f03ab8..7c5028ae7a58 100644 --- a/packages/feedback/test/widget/Form.test.ts +++ b/packages/feedback/test/widget/Form.test.ts @@ -104,42 +104,6 @@ describe('Form', () => { expect(messageInput.placeholder).toBe('What is the issue?!'); }); - it('submit is enabled if message is not empty', () => { - const formComponent = renderForm(); - - const message = formComponent.el.querySelector('[name="message"]') as HTMLTextAreaElement; - - message.value = 'Foo (message)'; - message.dispatchEvent(new KeyboardEvent('keyup')); - - message.value = ''; - message.dispatchEvent(new KeyboardEvent('keyup')); - }); - - it('submit is enabled if name is required and is not empty', () => { - const formComponent = renderForm({ isNameRequired: true }); - - const name = formComponent.el.querySelector('[name="name"]') as HTMLTextAreaElement; - - name.value = 'Foo Bar'; - name.dispatchEvent(new KeyboardEvent('keyup')); - - name.value = ''; - name.dispatchEvent(new KeyboardEvent('keyup')); - }); - - it('submit is enabled if email is required and is not empty', () => { - const formComponent = renderForm({ isEmailRequired: true }); - - const email = formComponent.el.querySelector('[name="email"]') as HTMLTextAreaElement; - - email.value = 'foo@example.org'; - email.dispatchEvent(new KeyboardEvent('keyup')); - - email.value = ''; - email.dispatchEvent(new KeyboardEvent('keyup')); - }); - it('can show error', () => { const formComponent = renderForm(); const errorEl = formComponent.el.querySelector('.form__error-container') as HTMLDivElement; diff --git a/packages/feedback/test/widget/createWidget.test.ts b/packages/feedback/test/widget/createWidget.test.ts index fb1809de7ecf..818c365105e1 100644 --- a/packages/feedback/test/widget/createWidget.test.ts +++ b/packages/feedback/test/widget/createWidget.test.ts @@ -171,6 +171,72 @@ describe('createWidget', () => { expect(shadow.querySelector('.success-message')).toBeNull(); }); + it('only submits feedback successfully when all required fields are filled', async () => { + const onSubmitSuccess = jest.fn(() => {}); + const { shadow, widget } = createShadowAndWidget({ + isNameRequired: true, + isEmailRequired: true, + onSubmitSuccess, + }); + + (sendFeedbackRequest as jest.Mock).mockImplementation(() => { + return true; + }); + widget.actor?.el?.dispatchEvent(new Event('click')); + + const nameEl = widget.dialog?.el?.querySelector('[name="name"]') as HTMLInputElement; + const emailEl = widget.dialog?.el?.querySelector('[name="email"]') as HTMLInputElement; + const messageEl = widget.dialog?.el?.querySelector('[name="message"]') as HTMLTextAreaElement; + + nameEl.value = ''; + emailEl.value = ''; + messageEl.value = ''; + + widget.dialog?.el?.querySelector('form')?.dispatchEvent(new Event('submit')); + expect(sendFeedbackRequest).toHaveBeenCalledTimes(0); + + // sendFeedbackRequest is async + await flushPromises(); + expect(onSubmitSuccess).toHaveBeenCalledTimes(0); + + nameEl.value = ''; + emailEl.value = ''; + messageEl.value = 'My feedback'; + + widget.dialog?.el?.querySelector('form')?.dispatchEvent(new Event('submit')); + expect(sendFeedbackRequest).toHaveBeenCalledTimes(0); + + // sendFeedbackRequest is async + await flushPromises(); + expect(onSubmitSuccess).toHaveBeenCalledTimes(0); + + nameEl.value = 'Jane Doe'; + emailEl.value = 'jane@example.com'; + 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', + }, + }); + + // sendFeedbackRequest is async + await flushPromises(); + expect(onSubmitSuccess).toHaveBeenCalledTimes(1); + + expect(widget.dialog).toBeUndefined(); + expect(shadow.querySelector('.success-message')?.textContent).toBe(SUCCESS_MESSAGE_TEXT); + + jest.runAllTimers(); + expect(shadow.querySelector('.success-message')).toBeNull(); + }); + it('submits feedback with error on request', async () => { const onSubmitError = jest.fn(() => {}); const { shadow, widget } = createShadowAndWidget({ From dc524c2c68ecfcd884d08bb281d1de6237f6afe2 Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Fri, 17 Nov 2023 15:27:27 -0500 Subject: [PATCH 04/18] typo --- packages/feedback/src/widget/createWidget.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/feedback/src/widget/createWidget.ts b/packages/feedback/src/widget/createWidget.ts index 42ebbd962a02..6f281178f79f 100644 --- a/packages/feedback/src/widget/createWidget.ts +++ b/packages/feedback/src/widget/createWidget.ts @@ -100,7 +100,7 @@ export function createWidget({ return; } - // Simple validation for now, just check for non-empty message if name required + // Simple validation for now, just check for non-empty message if email required if (options.isEmailRequired && !feedback.email) { dialog.showError('Please enter in an email before submitting!'); return; From d64c9e625bb4ea2fd54c15ea315349f28920b251 Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Fri, 17 Nov 2023 16:30:48 -0500 Subject: [PATCH 05/18] empty field validation --- packages/feedback/src/widget/createWidget.ts | 22 ++++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/feedback/src/widget/createWidget.ts b/packages/feedback/src/widget/createWidget.ts index 6f281178f79f..daf4d4fef1a1 100644 --- a/packages/feedback/src/widget/createWidget.ts +++ b/packages/feedback/src/widget/createWidget.ts @@ -88,21 +88,21 @@ export function createWidget({ return; } - // Simple validation for now, just check for non-empty message - if (!feedback.message) { - dialog.showError('Please enter in some feedback before submitting!'); - return; - } - - // Simple validation for now, just check for non-empty message if name required + // Simple validation for now, just check for non-empty required fields + const emptyField = []; if (options.isNameRequired && !feedback.name) { - dialog.showError('Please enter in a name before submitting!'); + emptyField.push(options.nameLabel); return; } - - // Simple validation for now, just check for non-empty message if email required if (options.isEmailRequired && !feedback.email) { - dialog.showError('Please enter in an email before submitting!'); + emptyField.push(options.emailLabel); + return; + } + if (!feedback.message) { + emptyField.push(options.messageLabel); + } + if (!emptyField.length) { + dialog.showError(`Please enter in the following required fields: ${emptyField.join(', ')}`); return; } From cd5893caf1e06860e82c5edc5698e1e1eda57211 Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Fri, 17 Nov 2023 16:37:57 -0500 Subject: [PATCH 06/18] remove returns --- packages/feedback/src/widget/createWidget.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/feedback/src/widget/createWidget.ts b/packages/feedback/src/widget/createWidget.ts index daf4d4fef1a1..b9486fd25af0 100644 --- a/packages/feedback/src/widget/createWidget.ts +++ b/packages/feedback/src/widget/createWidget.ts @@ -92,11 +92,9 @@ export function createWidget({ const emptyField = []; if (options.isNameRequired && !feedback.name) { emptyField.push(options.nameLabel); - return; } if (options.isEmailRequired && !feedback.email) { emptyField.push(options.emailLabel); - return; } if (!feedback.message) { emptyField.push(options.messageLabel); From 421e030402f71eff5fa7356ee830492ea5b3e8c7 Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Fri, 17 Nov 2023 16:37:57 -0500 Subject: [PATCH 07/18] remove returns --- packages/feedback/src/widget/createWidget.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/feedback/src/widget/createWidget.ts b/packages/feedback/src/widget/createWidget.ts index b9486fd25af0..5bf453011f3b 100644 --- a/packages/feedback/src/widget/createWidget.ts +++ b/packages/feedback/src/widget/createWidget.ts @@ -99,7 +99,7 @@ export function createWidget({ if (!feedback.message) { emptyField.push(options.messageLabel); } - if (!emptyField.length) { + if (emptyField.length != 0) { dialog.showError(`Please enter in the following required fields: ${emptyField.join(', ')}`); return; } From 332f8e73046f5b25199a2f2ed244b00a6796525b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=BDubom=C3=ADrIgonda?= <9303146+LubomirIgonda1@users.noreply.github.com> Date: Mon, 20 Nov 2023 17:44:45 +0100 Subject: [PATCH 08/18] fix(tracing-internal): Fix case when originalURL contain query params (#9531) This PR fix use case when original req url contain query params. For example: `api/v1/users/posts?param=1` ### Old behavior sentry build req._reconstructedRoute as: api/v1/users ### New behavior sentry build req._reconstructedRoute as: api/v1/users/posts --- .../src/node/integrations/express.ts | 4 +++- .../tracing-internal/test/node/express.test.ts | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/tracing-internal/src/node/integrations/express.ts b/packages/tracing-internal/src/node/integrations/express.ts index b8874ec6f27a..e09ff0902193 100644 --- a/packages/tracing-internal/src/node/integrations/express.ts +++ b/packages/tracing-internal/src/node/integrations/express.ts @@ -545,7 +545,9 @@ export function preventDuplicateSegments( reconstructedRoute?: string, layerPath?: string, ): string | undefined { - const originalUrlSplit = originalUrl?.split('/').filter(v => !!v); + // filter query params + const normalizeURL = stripUrlQueryAndFragment(originalUrl || ''); + const originalUrlSplit = normalizeURL?.split('/').filter(v => !!v); let tempCounter = 0; const currentOffset = reconstructedRoute?.split('/').filter(v => !!v).length || 0; const result = layerPath diff --git a/packages/tracing-internal/test/node/express.test.ts b/packages/tracing-internal/test/node/express.test.ts index 4b8d31fb2cdc..1631971d9863 100644 --- a/packages/tracing-internal/test/node/express.test.ts +++ b/packages/tracing-internal/test/node/express.test.ts @@ -35,6 +35,22 @@ describe('unit Test for preventDuplicateSegments', () => { const result1 = preventDuplicateSegments(originalUrl, reconstructedRoute, layerPath); expect(result1).toBe('1234'); }); + + it('should prevent duplicate segment v1 originalUrl with query param without trailing slash', () => { + const originalUrl = '/api/v1/1234?queryParam=123'; + const reconstructedRoute = '/api/v1'; + const layerPath = '/v1/1234'; + const result1 = preventDuplicateSegments(originalUrl, reconstructedRoute, layerPath); + expect(result1).toBe('1234'); + }); + + it('should prevent duplicate segment v1 originalUrl with query param with trailing slash', () => { + const originalUrl = '/api/v1/1234/?queryParam=123'; + const reconstructedRoute = '/api/v1'; + const layerPath = '/v1/1234'; + const result1 = preventDuplicateSegments(originalUrl, reconstructedRoute, layerPath); + expect(result1).toBe('1234'); + }); }); describe('preventDuplicateSegments should handle empty input gracefully', () => { it('Empty input values', () => { From 5fe5272e88a1fa9e72a47128317baeedb86a580a Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Mon, 20 Nov 2023 14:40:54 -0500 Subject: [PATCH 09/18] remove isAnonymous from feedback --- packages/feedback/src/integration.ts | 2 -- packages/feedback/src/types/index.ts | 5 --- packages/feedback/src/widget/Dialog.ts | 2 -- packages/feedback/src/widget/Form.ts | 12 +++---- packages/feedback/src/widget/createWidget.ts | 5 ++- packages/feedback/test/widget/Dialog.test.ts | 2 -- packages/feedback/test/widget/Form.test.ts | 31 ------------------- .../feedback/test/widget/createWidget.test.ts | 1 - 8 files changed, 6 insertions(+), 54 deletions(-) diff --git a/packages/feedback/src/integration.ts b/packages/feedback/src/integration.ts index 052d57353957..4a7bd2a7e10a 100644 --- a/packages/feedback/src/integration.ts +++ b/packages/feedback/src/integration.ts @@ -80,7 +80,6 @@ export class Feedback implements Integration { email: 'email', name: 'username', }, - isAnonymous = false, isEmailRequired = false, isNameRequired = false, @@ -120,7 +119,6 @@ export class Feedback implements Integration { id, showBranding, autoInject, - isAnonymous, isEmailRequired, isNameRequired, showEmail, diff --git a/packages/feedback/src/types/index.ts b/packages/feedback/src/types/index.ts index 5772e6e8176b..89965f017cdd 100644 --- a/packages/feedback/src/types/index.ts +++ b/packages/feedback/src/types/index.ts @@ -52,11 +52,6 @@ export interface FeedbackGeneralConfiguration { */ autoInject: boolean; - /** - * If true, will not collect user data (email/name). - */ - isAnonymous: boolean; - /** * Should the email field be required? */ diff --git a/packages/feedback/src/widget/Dialog.ts b/packages/feedback/src/widget/Dialog.ts index 35a4c43b7bce..84b4318b34d1 100644 --- a/packages/feedback/src/widget/Dialog.ts +++ b/packages/feedback/src/widget/Dialog.ts @@ -48,7 +48,6 @@ export function Dialog({ isNameRequired, isEmailRequired, colorScheme, - isAnonymous, defaultName, defaultEmail, onClosed, @@ -103,7 +102,6 @@ export function Dialog({ } = Form({ showEmail, showName, - isAnonymous, isEmailRequired, isNameRequired, diff --git a/packages/feedback/src/widget/Form.ts b/packages/feedback/src/widget/Form.ts index 50fd49fd9528..9b90cf547477 100644 --- a/packages/feedback/src/widget/Form.ts +++ b/packages/feedback/src/widget/Form.ts @@ -7,7 +7,6 @@ export interface FormComponentProps FeedbackInternalOptions, | 'showName' | 'showEmail' - | 'isAnonymous' | 'isNameRequired' | 'isEmailRequired' | Exclude @@ -59,7 +58,6 @@ export function Form({ showName, showEmail, - isAnonymous, isNameRequired, isEmailRequired, @@ -166,8 +164,7 @@ export function Form({ [ errorEl, - !isAnonymous && - showName && + showName && createElement( 'label', { @@ -184,10 +181,9 @@ export function Form({ nameEl, ], ), - !isAnonymous && !showName && nameEl, + !showName && nameEl, - !isAnonymous && - showEmail && + showEmail && createElement( 'label', { @@ -204,7 +200,7 @@ export function Form({ emailEl, ], ), - !isAnonymous && !showEmail && emailEl, + !showEmail && emailEl, createElement( 'label', diff --git a/packages/feedback/src/widget/createWidget.ts b/packages/feedback/src/widget/createWidget.ts index 5bf453011f3b..1a1077b0c257 100644 --- a/packages/feedback/src/widget/createWidget.ts +++ b/packages/feedback/src/widget/createWidget.ts @@ -99,7 +99,7 @@ export function createWidget({ if (!feedback.message) { emptyField.push(options.messageLabel); } - if (emptyField.length != 0) { + if (emptyField.length > 0) { dialog.showError(`Please enter in the following required fields: ${emptyField.join(', ')}`); return; } @@ -159,7 +159,7 @@ export function createWidget({ return; } - const userKey = !options.isAnonymous && options.useSentryUser; + const userKey = options.useSentryUser; const scope = getCurrentHub().getScope(); const user = scope && scope.getUser(); @@ -168,7 +168,6 @@ export function createWidget({ showBranding: options.showBranding, showName: options.showName || options.isNameRequired, showEmail: options.showEmail || options.isEmailRequired, - isAnonymous: options.isAnonymous, isNameRequired: options.isNameRequired, isEmailRequired: options.isEmailRequired, formTitle: options.formTitle, diff --git a/packages/feedback/test/widget/Dialog.test.ts b/packages/feedback/test/widget/Dialog.test.ts index e601f4783d96..69a333f69813 100644 --- a/packages/feedback/test/widget/Dialog.test.ts +++ b/packages/feedback/test/widget/Dialog.test.ts @@ -9,7 +9,6 @@ function renderDialog({ showName = true, showEmail = true, showBranding = false, - isAnonymous = false, isNameRequired = false, isEmailRequired = false, formTitle = 'Feedback', @@ -29,7 +28,6 @@ function renderDialog({ return Dialog({ formTitle, - isAnonymous, showName, showEmail, isNameRequired, diff --git a/packages/feedback/test/widget/Form.test.ts b/packages/feedback/test/widget/Form.test.ts index 7c5028ae7a58..7b45cabd8503 100644 --- a/packages/feedback/test/widget/Form.test.ts +++ b/packages/feedback/test/widget/Form.test.ts @@ -8,7 +8,6 @@ type NonNullableFields = { function renderForm({ showName = true, showEmail = true, - isAnonymous = false, isNameRequired = false, isEmailRequired = false, defaultName = 'Foo Bar', @@ -24,7 +23,6 @@ function renderForm({ ...rest }: Partial = {}) { return Form({ - isAnonymous, showName, showEmail, isNameRequired, @@ -138,33 +136,4 @@ describe('Form', () => { name: 'Foo Bar', }); }); - - it('does not show name or email inputs for anonymous mode', () => { - const onSubmit = jest.fn(); - const formComponent = renderForm({ - isNameRequired: true, - isEmailRequired: true, - isAnonymous: true, - onSubmit, - }); - const submitEvent = new Event('submit'); - - expect(formComponent.el).toBeInstanceOf(HTMLFormElement); - const nameInput = formComponent.el.querySelector('[name="name"][type="text"]') as HTMLInputElement; - const emailInput = formComponent.el.querySelector('[name="email"][type="text"]') as HTMLInputElement; - expect(nameInput).toBeNull(); - expect(emailInput).toBeNull(); - expect(formComponent.el.querySelector('[name="message"]')).not.toBeNull(); - - const message = formComponent.el.querySelector('[name="message"]') as HTMLTextAreaElement; - message.value = 'Foo (message)'; - message.dispatchEvent(new KeyboardEvent('keyup')); - - formComponent.el.dispatchEvent(submitEvent); - expect(onSubmit).toHaveBeenCalledWith({ - email: '', - message: 'Foo (message)', - name: '', - }); - }); }); diff --git a/packages/feedback/test/widget/createWidget.test.ts b/packages/feedback/test/widget/createWidget.test.ts index 818c365105e1..8502726612d3 100644 --- a/packages/feedback/test/widget/createWidget.test.ts +++ b/packages/feedback/test/widget/createWidget.test.ts @@ -30,7 +30,6 @@ const DEFAULT_OPTIONS = { email: 'email', name: 'username', }, - isAnonymous: false, isEmailRequired: false, isNameRequired: false, From 205c1c94a0ae7d9f47404e366cc34a63dd03c000 Mon Sep 17 00:00:00 2001 From: Josh Kelley Date: Mon, 20 Nov 2023 14:54:47 -0500 Subject: [PATCH 10/18] fix(node): Improve error handling and shutdown handling for ANR (#9548) If communicating via debugger to generate a stack trace fails, then fall back to a regular event, instead of throwing an uncaught exception. If the parent process exits, exit the ANR child. --- packages/node/src/anr/index.ts | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/packages/node/src/anr/index.ts b/packages/node/src/anr/index.ts index caa384b3928f..549b962d4b70 100644 --- a/packages/node/src/anr/index.ts +++ b/packages/node/src/anr/index.ts @@ -215,15 +215,21 @@ function handleChildProcess(options: Options): void { async function watchdogTimeout(): Promise { log('Watchdog timeout'); - const pauseAndCapture = await debuggerPause; - - if (pauseAndCapture) { - log('Pausing debugger to capture stack trace'); - pauseAndCapture(); - } else { - log('Capturing event'); - sendAnrEvent(); + + try { + const pauseAndCapture = await debuggerPause; + + if (pauseAndCapture) { + log('Pausing debugger to capture stack trace'); + pauseAndCapture(); + return; + } + } catch (_) { + // ignore } + + log('Capturing event'); + sendAnrEvent(); } const { poll } = watchdogTimer(createHrTimer, options.pollInterval, options.anrThreshold, watchdogTimeout); @@ -234,6 +240,10 @@ function handleChildProcess(options: Options): void { } poll(); }); + process.on('disconnect', () => { + // Parent process has exited. + process.exit(); + }); } /** From 76b136f339007a795ee97682292db4c976b410f3 Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Mon, 20 Nov 2023 15:11:14 -0500 Subject: [PATCH 11/18] update README --- packages/feedback/README.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/feedback/README.md b/packages/feedback/README.md index 53a4578d40b3..3772b65f95d6 100644 --- a/packages/feedback/README.md +++ b/packages/feedback/README.md @@ -65,7 +65,8 @@ The following options can be configured as options to the integration, in `new F | --------- | ------- | ------- | ----------- | | `showName` | `boolean` | `true` | Displays the name field on the feedback form, however will still capture the name (if available) from Sentry SDK context. | | `showEmail` | `boolean` | `true` | Displays the email field on the feedback form, however will still capture the email (if available) from Sentry SDK context. | -| `isAnonymous` | `boolean` | `false` | Hides both name and email fields and does not use Sentry SDK's user context. | +| `isNameRequired` | `boolean` | `false` | Requires the name field on the feedback form to be filled in. | +| `isEmailRequired` | `boolean` | `false` | Requires the email field on the feedback form to be filled in. | | `useSentryUser` | `Record` | `{ email: 'email', name: 'username'}` | Map of the `email` and `name` fields to the corresponding Sentry SDK user fields that were called with `Sentry.setUser`. | By default the Feedback integration will attempt to fill in the name/email fields if you have set a user context via [`Sentry.setUser`](https://docs.sentry.io/platforms/javascript/enriching-events/identify-user/). By default it expects the email and name fields to be `email` and `username`. Below is an example configuration with non-default user fields. @@ -133,7 +134,7 @@ Colors can be customized via the Feedback constructor or by defining CSS variabl | `submitForegroundHover` | `--submit-foreground-hover` | `#ffffff` | `#ffffff` | Foreground color for the submit button when hovering | | `cancelBackground` | `--cancel-background` | `transparent` | `transparent` | Background color for the cancel button | | `cancelBackgroundHover` | `--cancel-background-hover` | `var(--background-hover)` | `var(--background-hover)` | Background color when hovering over the cancel button | -| `cancelBorder` | `--cancel-border` | `var(--border)` | `var(--border)` | Border style for the cancel button | +| `cancelBorder` | `--cancel-border` | `var(--border)` | `var(--border)` | Border style for the cancel button | | `cancelOutlineFocus` | `--cancel-outline-focus` | `var(--input-outline-focus)` | `var(--input-outline-focus)` | Outline color for the cancel button, in the focused state | | `cancelForeground` | `--cancel-foreground` | `var(--foreground)` | `var(--foreground)` | Foreground color for the cancel button | | `cancelForegroundHover` | `--cancel-foreground-hover` | `var(--foreground)` | `var(--foreground)` | Foreground color for the cancel button when hovering | @@ -270,7 +271,7 @@ document.getElementById('my-feedback-form').addEventListener('submit', (event) = Note: The following instructions are to be followed in the Sentry product. -If you have Sentry's default issue alert ("Alert me on every new issue") turned on for the project you are setting up User Feedback on, no action is required to have alerting on each user feedback report. +If you have Sentry's default issue alert ("Alert me on every new issue") turned on for the project you are setting up User Feedback on, no action is required to have alerting on each user feedback report. If you don't have Sentry's default issue alert turned on, follow these steps: From f24ea887a842494abf191da99f7e79a6ea436f31 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Mon, 20 Nov 2023 17:51:05 -0330 Subject: [PATCH 12/18] ref(feedback): Refactor attaching replay id to feedback event to use hooks (#9588) Add a `beforeSendFeedback` hook that replay listens to, to attach replayId to the feedback event. --- packages/core/src/baseclient.ts | 10 +++ packages/feedback/src/sendFeedback.ts | 31 ++++------ .../feedback/src/util/prepareFeedbackEvent.ts | 1 + .../feedback/src/util/sendFeedbackRequest.ts | 14 +++-- .../feedback/test/widget/createWidget.test.ts | 62 ++++++++++--------- .../replay/src/util/addGlobalListeners.ts | 11 ++++ packages/types/src/client.ts | 18 ++++++ 7 files changed, 95 insertions(+), 52 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index c811c4f827a3..cd440b376655 100644 --- 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 implements Client { /** @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 implements Client { /** @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 index 4f1b79761a96..ac60415d0462 100644 --- 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 { - const client = getCurrentHub().getClient(); - 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 index 3ef01fb500c6..9b1b0f9c6e8b 100644 --- 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 index 30e19bf0971f..b8ec16a15401 100644 --- 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 { +export async function sendFeedbackRequest( + { feedback: { message, email, name, source, url } }: SendFeedbackData, + { includeReplay = true }: SendFeedbackOptions = {}, +): Promise { 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 index 8502726612d3..1776e14f5e21 100644 --- 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 index 1f7945565524..bf133c3e2130 100644 --- 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 index 8aeabaa6cc8d..33fa749eb379 100644 --- 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 { */ 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 { */ 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 */ } From b46a1bb6eaf9fd22362a9fdf46cb8bab6cc5ec3b Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Mon, 20 Nov 2023 18:52:09 -0330 Subject: [PATCH 13/18] ref(feedback): Do not import window from browser pkg to avoid circular import (#9604) We will be exporting the Feedback package from the `@sentry/browser` package, so to avoid circular imports when we do, we should remove imports from `@sentry/browser` (this is what we did in Replay) --- packages/feedback/package.json | 1 - packages/feedback/src/constants.ts | 7 +++++++ packages/feedback/src/integration.ts | 2 +- packages/feedback/src/widget/Icon.ts | 3 +-- packages/feedback/src/widget/Logo.ts | 3 +-- packages/feedback/src/widget/SuccessIcon.ts | 3 +-- packages/feedback/src/widget/createShadowHost.ts | 2 +- packages/feedback/src/widget/util/createElement.ts | 2 +- 8 files changed, 13 insertions(+), 10 deletions(-) diff --git a/packages/feedback/package.json b/packages/feedback/package.json index 522fcb5b4b90..0f28b1736ab7 100644 --- a/packages/feedback/package.json +++ b/packages/feedback/package.json @@ -23,7 +23,6 @@ "access": "public" }, "dependencies": { - "@sentry/browser": "7.81.0", "@sentry/core": "7.81.0", "@sentry/types": "7.81.0", "@sentry/utils": "7.81.0" diff --git a/packages/feedback/src/constants.ts b/packages/feedback/src/constants.ts index ea0db22525f4..48f699408762 100644 --- a/packages/feedback/src/constants.ts +++ b/packages/feedback/src/constants.ts @@ -1,3 +1,10 @@ +import { GLOBAL_OBJ } from '@sentry/utils'; + +// exporting a separate copy of `WINDOW` rather than exporting the one from `@sentry/browser` +// prevents the browser package from being bundled in the CDN bundle, and avoids a +// circular dependency between the browser and feedback packages +export const WINDOW = GLOBAL_OBJ as typeof GLOBAL_OBJ & Window; + const LIGHT_BACKGROUND = '#ffffff'; const INHERIT = 'inherit'; const SUBMIT_COLOR = 'rgba(108, 95, 199, 1)'; diff --git a/packages/feedback/src/integration.ts b/packages/feedback/src/integration.ts index 4a7bd2a7e10a..cb27042c20fc 100644 --- a/packages/feedback/src/integration.ts +++ b/packages/feedback/src/integration.ts @@ -1,4 +1,3 @@ -import { WINDOW } from '@sentry/browser'; import type { Integration } from '@sentry/types'; import { isBrowser, logger } from '@sentry/utils'; @@ -15,6 +14,7 @@ import { NAME_PLACEHOLDER, SUBMIT_BUTTON_LABEL, SUCCESS_MESSAGE_TEXT, + WINDOW, } from './constants'; import type { FeedbackInternalOptions, FeedbackWidget, OptionalFeedbackConfiguration } from './types'; import { mergeOptions } from './util/mergeOptions'; diff --git a/packages/feedback/src/widget/Icon.ts b/packages/feedback/src/widget/Icon.ts index 998eaef1ebb8..29b2d25bee8d 100644 --- a/packages/feedback/src/widget/Icon.ts +++ b/packages/feedback/src/widget/Icon.ts @@ -1,5 +1,4 @@ -import { WINDOW } from '@sentry/browser'; - +import { WINDOW } from '../constants'; import { setAttributesNS } from '../util/setAttributesNS'; const SIZE = 20; diff --git a/packages/feedback/src/widget/Logo.ts b/packages/feedback/src/widget/Logo.ts index b8c7c72b1d39..17333bda87ed 100644 --- a/packages/feedback/src/widget/Logo.ts +++ b/packages/feedback/src/widget/Logo.ts @@ -1,5 +1,4 @@ -import { WINDOW } from '@sentry/browser'; - +import { WINDOW } from '../constants'; import type { FeedbackInternalOptions } from '../types'; import { setAttributesNS } from '../util/setAttributesNS'; diff --git a/packages/feedback/src/widget/SuccessIcon.ts b/packages/feedback/src/widget/SuccessIcon.ts index 672954eae864..6092481672b9 100644 --- a/packages/feedback/src/widget/SuccessIcon.ts +++ b/packages/feedback/src/widget/SuccessIcon.ts @@ -1,5 +1,4 @@ -import { WINDOW } from '@sentry/browser'; - +import { WINDOW } from '../constants'; import { setAttributesNS } from '../util/setAttributesNS'; const WIDTH = 16; diff --git a/packages/feedback/src/widget/createShadowHost.ts b/packages/feedback/src/widget/createShadowHost.ts index bb8c0643900a..a418906d4b5a 100644 --- a/packages/feedback/src/widget/createShadowHost.ts +++ b/packages/feedback/src/widget/createShadowHost.ts @@ -1,6 +1,6 @@ -import { WINDOW } from '@sentry/browser'; import { logger } from '@sentry/utils'; +import { WINDOW } from '../constants'; import type { FeedbackInternalOptions } from '../types'; import { createDialogStyles } from './Dialog.css'; import { createMainStyles } from './Main.css'; diff --git a/packages/feedback/src/widget/util/createElement.ts b/packages/feedback/src/widget/util/createElement.ts index bf5f81868d68..e070e9a9553f 100644 --- a/packages/feedback/src/widget/util/createElement.ts +++ b/packages/feedback/src/widget/util/createElement.ts @@ -1,4 +1,4 @@ -import { WINDOW } from '@sentry/browser'; +import { WINDOW } from '../../constants'; /** * Helper function to create an element. Could be used as a JSX factory From 4b8d7554a3ed44c74be23c6768e73d86715ab59a Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Mon, 20 Nov 2023 19:28:29 -0600 Subject: [PATCH 14/18] chore: Add @sentry/deno to issue template (#9610) --- .github/ISSUE_TEMPLATE/bug.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/ISSUE_TEMPLATE/bug.yml b/.github/ISSUE_TEMPLATE/bug.yml index 044efc2b9696..61c24641c292 100644 --- a/.github/ISSUE_TEMPLATE/bug.yml +++ b/.github/ISSUE_TEMPLATE/bug.yml @@ -35,6 +35,7 @@ body: - '@sentry/angular' - '@sentry/angular-ivy' - '@sentry/bun' + - '@sentry/deno' - '@sentry/ember' - '@sentry/gatsby' - '@sentry/nextjs' From 48e2337d5d88ccee3e6e2a1be90f4f213ebcb8c2 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 21 Nov 2023 03:43:18 -0500 Subject: [PATCH 15/18] fix(deno): Make sure files get published (#9611) --- packages/deno/package.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/deno/package.json b/packages/deno/package.json index d4ff30a59585..c7a5e844fd3b 100644 --- a/packages/deno/package.json +++ b/packages/deno/package.json @@ -6,14 +6,14 @@ "homepage": "https://github.com/getsentry/sentry-javascript/tree/master/packages/deno", "author": "Sentry", "license": "MIT", - "module": "build/index.js", + "module": "build/index.mjs", "types": "build/index.d.ts", "publishConfig": { "access": "public" }, "files": [ - "index.js", - "index.js.map", + "index.mjs", + "index.mjs.map", "index.d.ts" ], "dependencies": { From 2f17bb1ec69cf0f3da5117934021c8f14243f98b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 21 Nov 2023 03:58:50 -0500 Subject: [PATCH 16/18] fix(astro): Remove method from span op (#9603) Although adding method is nice to span op, it duplicates information that is already in span description. In addition, we want to reduce the total list of span ops as much as possible for cardinality reasons, `http.server` + span data about method is good enough to understand intent here. --- packages/astro/src/server/middleware.ts | 2 +- packages/astro/test/server/middleware.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts index c04618cad33f..b5b7aa7d8c71 100644 --- a/packages/astro/src/server/middleware.ts +++ b/packages/astro/src/server/middleware.ts @@ -83,7 +83,7 @@ export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseH const res = await startSpan( { name: `${method} ${interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params)}`, - op: `http.server.${method.toLowerCase()}`, + op: 'http.server', origin: 'auto.http.astro', status: 'ok', ...traceparentData, diff --git a/packages/astro/test/server/middleware.test.ts b/packages/astro/test/server/middleware.test.ts index 058982f06cc6..59ab8c18a3c4 100644 --- a/packages/astro/test/server/middleware.test.ts +++ b/packages/astro/test/server/middleware.test.ts @@ -47,7 +47,7 @@ describe('sentryMiddleware', () => { source: 'route', }, name: 'GET /users/[id]/details', - op: 'http.server.get', + op: 'http.server', origin: 'auto.http.astro', status: 'ok', }, From b8c46eae44165b2ea96749f3423fddaed6a42c79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9D=9E=E5=87=A1=E5=B0=91=E5=B9=B4?= Date: Tue, 21 Nov 2023 17:03:08 +0800 Subject: [PATCH 17/18] fix(nextjs): Use `globalThis` instead of `global` in edge runtime (#9612) Co-authored-by: Luca Forstner --- packages/nextjs/src/config/loaders/valueInjectionLoader.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/loaders/valueInjectionLoader.ts b/packages/nextjs/src/config/loaders/valueInjectionLoader.ts index 13f94e7128e4..5bc84baf95ce 100644 --- a/packages/nextjs/src/config/loaders/valueInjectionLoader.ts +++ b/packages/nextjs/src/config/loaders/valueInjectionLoader.ts @@ -19,7 +19,8 @@ export default function valueInjectionLoader(this: LoaderThis, us this.cacheable(false); // Define some global proxy that works on server and on the browser. - let injectedCode = 'var _sentryCollisionFreeGlobalObject = typeof window === "undefined" ? global : window;\n'; + let injectedCode = + 'var _sentryCollisionFreeGlobalObject = typeof window != "undefined" ? window : typeof global != "undefined" ? global : typeof self != "undefined" ? self : {};\n'; Object.entries(values).forEach(([key, value]) => { injectedCode += `_sentryCollisionFreeGlobalObject["${key}"] = ${JSON.stringify(value)};\n`; From aa459aa6619c4afedda549e775fb86916720906a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 21 Nov 2023 09:09:19 +0000 Subject: [PATCH 18/18] meta(changelog): Update changelog for 7.81.1 --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cf73331536f..e2b8e6c9f0f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +## 7.81.1 + +- fix(astro): Remove method from span op (#9603) +- fix(deno): Make sure files get published (#9611) +- fix(nextjs): Use `globalThis` instead of `global` in edge runtime (#9612) +- fix(node): Improve error handling and shutdown handling for ANR (#9548) +- fix(tracing-internal): Fix case when originalURL contain query params (#9531) + +Work in this release contributed by @powerfulyang, @LubomirIgonda1, @joshkel, and @alexgleason. Thank you for your contributions! + ## 7.81.0 ### Important Changes