From 2bda59fa4c2a52ea17d608ab8e41b1ce995de0df Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Thu, 10 Apr 2025 15:10:00 -0700 Subject: [PATCH 1/5] fix(feedback): Fix an issue where, if removeFromDom() is called it might throw --- packages/feedback/src/core/components/Actor.ts | 14 ++++++++++++-- packages/feedback/src/modal/integration.tsx | 12 ++++++++++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/packages/feedback/src/core/components/Actor.ts b/packages/feedback/src/core/components/Actor.ts index 3ba04e85ddd1..505e721a3838 100644 --- a/packages/feedback/src/core/components/Actor.ts +++ b/packages/feedback/src/core/components/Actor.ts @@ -1,6 +1,8 @@ +import { DEBUG_BUILD } from 'src/util/debug-build'; import { DOCUMENT, TRIGGER_LABEL } from '../../constants'; import { createActorStyles } from './Actor.css'; import { FeedbackIcon } from './FeedbackIcon'; +import { logger } from '@sentry/core'; export interface ActorProps { triggerLabel: string; @@ -46,8 +48,16 @@ export function Actor({ triggerLabel, triggerAriaLabel, shadow, styleNonce }: Ac shadow.appendChild(el); }, removeFromDom(): void { - shadow.removeChild(el); - shadow.removeChild(style); + try { + el.remove(); + style.remove(); + } catch { + DEBUG_BUILD && + logger.error( + '[Feedback] Error when trying to remove Actor from the DOM. It is not appended to the DOM yet!', + ); + throw new Error('[Feedback] Actor is not appended to DOM, nothing to remove.'); + } }, show(): void { el.ariaHidden = 'false'; diff --git a/packages/feedback/src/modal/integration.tsx b/packages/feedback/src/modal/integration.tsx index f4b228d814b6..4e24ee854c71 100644 --- a/packages/feedback/src/modal/integration.tsx +++ b/packages/feedback/src/modal/integration.tsx @@ -5,6 +5,8 @@ import * as hooks from 'preact/hooks'; import { DOCUMENT } from '../constants'; import { Dialog } from './components/Dialog'; import { createDialogStyles } from './components/Dialog.css'; +import { DEBUG_BUILD } from 'src/util/debug-build'; +import { logger } from '@sentry/core'; function getUser(): User | undefined { const currentUser = getCurrentScope().getUser(); @@ -44,8 +46,14 @@ export const feedbackModalIntegration = ((): FeedbackModalIntegration => { } }, removeFromDom(): void { - shadowRoot.removeChild(el); - shadowRoot.removeChild(style); + try { + el.remove(); + style.remove(); + } catch { + DEBUG_BUILD && + logger.error('[Feedback] Error when trying to remove Modal from the DOM. It is not appended to the DOM yet!'); + throw new Error('[Feedback] Modal is not appended to DOM, nothing to remove.'); + } DOCUMENT.body.style.overflow = originalOverflow; }, open() { From 18c9875dad02d49bfe269f58e8445184aa235a19 Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Mon, 14 Apr 2025 13:42:47 -0700 Subject: [PATCH 2/5] fix import --- packages/feedback/src/core/components/Actor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/feedback/src/core/components/Actor.ts b/packages/feedback/src/core/components/Actor.ts index 505e721a3838..f57935555672 100644 --- a/packages/feedback/src/core/components/Actor.ts +++ b/packages/feedback/src/core/components/Actor.ts @@ -1,4 +1,4 @@ -import { DEBUG_BUILD } from 'src/util/debug-build'; +import { DEBUG_BUILD } from '../../util/debug-build'; import { DOCUMENT, TRIGGER_LABEL } from '../../constants'; import { createActorStyles } from './Actor.css'; import { FeedbackIcon } from './FeedbackIcon'; From 050ff862b128e4e819b14cbda7b2fbf57eeb4f2c Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Mon, 14 Apr 2025 13:57:24 -0700 Subject: [PATCH 3/5] no need to ever throw if we try to remove and its already removed --- .../test/core/components/Actor.test.ts | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/feedback/test/core/components/Actor.test.ts b/packages/feedback/test/core/components/Actor.test.ts index 06782f10393a..d238b5c5019f 100644 --- a/packages/feedback/test/core/components/Actor.test.ts +++ b/packages/feedback/test/core/components/Actor.test.ts @@ -63,4 +63,24 @@ describe('Actor', () => { expect(actorAria.el.textContent).toBe('Button'); expect(actorAria.el.ariaLabel).toBe('Aria'); }); + + it('does not throw if removeFromDom() is called when it is not mounted', () => { + const feedbackIntegration = buildFeedbackIntegration({ + lazyLoadIntegration: vi.fn(), + }); + + const configuredIntegration = feedbackIntegration({}); + mockSdk({ + sentryOptions: { + integrations: [configuredIntegration], + }, + }); + + const feedback = getFeedback(); + + const actorComponent = feedback!.createWidget(); + + expect(() => actorComponent.removeFromDom()).not.toThrowError(); + expect(() => actorComponent.removeFromDom()).not.toThrowError(); + }); }); From 1d9037283dbad7e4b3356f21d35e190078cf319b Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Mon, 14 Apr 2025 13:57:46 -0700 Subject: [PATCH 4/5] no need to ever throw if we try to remove and its already removed --- packages/feedback/src/core/components/Actor.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/feedback/src/core/components/Actor.ts b/packages/feedback/src/core/components/Actor.ts index f57935555672..2bfc67ddbc8a 100644 --- a/packages/feedback/src/core/components/Actor.ts +++ b/packages/feedback/src/core/components/Actor.ts @@ -56,7 +56,6 @@ export function Actor({ triggerLabel, triggerAriaLabel, shadow, styleNonce }: Ac logger.error( '[Feedback] Error when trying to remove Actor from the DOM. It is not appended to the DOM yet!', ); - throw new Error('[Feedback] Actor is not appended to DOM, nothing to remove.'); } }, show(): void { From b85449be905d3402a7fb4a0d3f553b9f0ff45be8 Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Mon, 14 Apr 2025 13:59:37 -0700 Subject: [PATCH 5/5] remove() doesnt throw --- packages/feedback/src/core/components/Actor.ts | 13 ++----------- packages/feedback/src/modal/integration.tsx | 12 ++---------- 2 files changed, 4 insertions(+), 21 deletions(-) diff --git a/packages/feedback/src/core/components/Actor.ts b/packages/feedback/src/core/components/Actor.ts index 2bfc67ddbc8a..ba90e9e116ec 100644 --- a/packages/feedback/src/core/components/Actor.ts +++ b/packages/feedback/src/core/components/Actor.ts @@ -1,8 +1,6 @@ -import { DEBUG_BUILD } from '../../util/debug-build'; import { DOCUMENT, TRIGGER_LABEL } from '../../constants'; import { createActorStyles } from './Actor.css'; import { FeedbackIcon } from './FeedbackIcon'; -import { logger } from '@sentry/core'; export interface ActorProps { triggerLabel: string; @@ -48,15 +46,8 @@ export function Actor({ triggerLabel, triggerAriaLabel, shadow, styleNonce }: Ac shadow.appendChild(el); }, removeFromDom(): void { - try { - el.remove(); - style.remove(); - } catch { - DEBUG_BUILD && - logger.error( - '[Feedback] Error when trying to remove Actor from the DOM. It is not appended to the DOM yet!', - ); - } + el.remove(); + style.remove(); }, show(): void { el.ariaHidden = 'false'; diff --git a/packages/feedback/src/modal/integration.tsx b/packages/feedback/src/modal/integration.tsx index 4e24ee854c71..d08d58b99a9e 100644 --- a/packages/feedback/src/modal/integration.tsx +++ b/packages/feedback/src/modal/integration.tsx @@ -5,8 +5,6 @@ import * as hooks from 'preact/hooks'; import { DOCUMENT } from '../constants'; import { Dialog } from './components/Dialog'; import { createDialogStyles } from './components/Dialog.css'; -import { DEBUG_BUILD } from 'src/util/debug-build'; -import { logger } from '@sentry/core'; function getUser(): User | undefined { const currentUser = getCurrentScope().getUser(); @@ -46,14 +44,8 @@ export const feedbackModalIntegration = ((): FeedbackModalIntegration => { } }, removeFromDom(): void { - try { - el.remove(); - style.remove(); - } catch { - DEBUG_BUILD && - logger.error('[Feedback] Error when trying to remove Modal from the DOM. It is not appended to the DOM yet!'); - throw new Error('[Feedback] Modal is not appended to DOM, nothing to remove.'); - } + el.remove(); + style.remove(); DOCUMENT.body.style.overflow = originalOverflow; }, open() {