From f975f365646cdd327539a732bf48aa13a239791c Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Thu, 23 Apr 2026 23:04:39 -0400 Subject: [PATCH] feat(feedback)!: always reject sendFeedback with an Error Previously, sendFeedback's async rejection paths (timeout, 403, generic transport error) rejected with a plain string, while the sync-throw paths (empty message, no client) threw an Error. Unify on Error for all paths so consumers can rely on a consistent rejection shape. BREAKING CHANGE: sendFeedback now rejects with an Error instead of a string. The message text is unchanged and is still customizable via the `errorMessages` hint, so consumers reading it should switch from the rejection value itself to `error.message`. --- MIGRATION.md | 16 +++++++++ packages/feedback/src/core/sendFeedback.ts | 8 ++--- .../feedback/src/modal/components/Form.tsx | 2 +- .../feedback/src/util/createFeedbackError.ts | 2 +- .../feedback/test/core/sendFeedback.test.ts | 35 ++++++++++++------- 5 files changed, 44 insertions(+), 19 deletions(-) diff --git a/MIGRATION.md b/MIGRATION.md index 1b5c54112e21..4219e8e1b134 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -613,6 +613,22 @@ on casing, or use `tracePropagationTargets` in combination with a more specific As part of this, the `g` and `y` flags are ignored on `tracePropagationTargets` regular expressions. These flags made matching stateful via `lastIndex`, so a target like `/myApi\.com/g` previously matched only every other request. +### `sendFeedback` rejects with an `Error` + +Affected SDKs: All SDKs running in the browser. + +`Sentry.sendFeedback()` now rejects with an `Error` in all cases. Previously it rejected with a plain string when the request timed out, was rejected with a 403, or otherwise failed to send, while the synchronous validation paths (empty message, no client configured) already threw an `Error`. The message text itself is unchanged, and is still customizable through the `errorMessages` hint, so read it off `error.message`: + +```js +try { + await Sentry.sendFeedback({ message: 'Hello' }); +} catch (error) { + // v10: a string on send failures, an Error on validation failures + // v11: always an Error + console.log(error.message); +} +``` + ### Span attribute changes Affected SDKs: All SDKs. diff --git a/packages/feedback/src/core/sendFeedback.ts b/packages/feedback/src/core/sendFeedback.ts index 204cf1cfa6c7..35c829ab0869 100644 --- a/packages/feedback/src/core/sendFeedback.ts +++ b/packages/feedback/src/core/sendFeedback.ts @@ -8,7 +8,7 @@ import type { } from '@sentry/core'; import { captureFeedback, getClient, getCurrentScope, getLocationHref } from '@sentry/core'; import { FEEDBACK_API_SOURCE } from '../constants'; -import { createFeedbackError, resolveFeedbackErrorMessage } from '../util/createFeedbackError'; +import { createFeedbackError } from '../util/createFeedbackError'; /** * Public API to send a Feedback item to Sentry @@ -47,7 +47,7 @@ export const sendFeedback: SendFeedback = ( // After 30s, we want to clear anyhow const timeout = setTimeout(() => { cleanup(); - reject(resolveFeedbackErrorMessage('ERROR_TIMEOUT', errorMessages)); + reject(createFeedbackError('ERROR_TIMEOUT', errorMessages)); }, 30_000); const cleanup = client.on('afterSendEvent', (event: Event, response: TransportMakeRequestResponse) => { @@ -64,10 +64,10 @@ export const sendFeedback: SendFeedback = ( } if (response?.statusCode === 403) { - return reject(resolveFeedbackErrorMessage('ERROR_FORBIDDEN', errorMessages)); + return reject(createFeedbackError('ERROR_FORBIDDEN', errorMessages)); } - return reject(resolveFeedbackErrorMessage('ERROR_GENERIC', errorMessages)); + return reject(createFeedbackError('ERROR_GENERIC', errorMessages)); }); }); }; diff --git a/packages/feedback/src/modal/components/Form.tsx b/packages/feedback/src/modal/components/Form.tsx index ee4b06b25d88..48436b947442 100644 --- a/packages/feedback/src/modal/components/Form.tsx +++ b/packages/feedback/src/modal/components/Form.tsx @@ -131,7 +131,7 @@ export function Form({ onSubmitSuccess(data, eventId); } catch (error) { DEBUG_BUILD && debug.error(error); - const err = error instanceof Error ? error : new Error(String(error)); + const err = error as Error; setError(err.message); onSubmitError(err); } diff --git a/packages/feedback/src/util/createFeedbackError.ts b/packages/feedback/src/util/createFeedbackError.ts index d7c2c4100f3e..93d15ba88752 100644 --- a/packages/feedback/src/util/createFeedbackError.ts +++ b/packages/feedback/src/util/createFeedbackError.ts @@ -15,7 +15,7 @@ const DEFAULT_MESSAGES: Record = { ERROR_GENERIC: ERROR_GENERIC_TEXT, }; -export function resolveFeedbackErrorMessage(code: FeedbackErrorCode, messages?: FeedbackErrorMessages): string { +function resolveFeedbackErrorMessage(code: FeedbackErrorCode, messages?: FeedbackErrorMessages): string { return messages?.[code] ?? DEFAULT_MESSAGES[code]; } diff --git a/packages/feedback/test/core/sendFeedback.test.ts b/packages/feedback/test/core/sendFeedback.test.ts index 8b345a4ea109..4ada5c2617e0 100644 --- a/packages/feedback/test/core/sendFeedback.test.ts +++ b/packages/feedback/test/core/sendFeedback.test.ts @@ -33,6 +33,18 @@ describe('sendFeedback', () => { patchedDecoder && delete global.window.TextDecoder; }); + // `sendFeedback` always signals failure with an `Error`, never a bare string. A `toThrow(text)` + // assertion alone also passes for a thrown/rejected string, so assert the shape explicitly too. + async function expectRejectsWithError(promise: Promise, message: string): Promise { + await expect(promise).rejects.toBeInstanceOf(Error); + await expect(promise).rejects.toThrow(message); + } + + function expectThrowsWithError(fn: () => unknown, message: string): void { + expect(fn).toThrow(Error); + expect(fn).toThrow(message); + } + it('sends feedback with minimal options', async () => { mockSdk(); const mockTransport = vi.spyOn(getClient()!.getTransport()!, 'send'); @@ -269,7 +281,7 @@ describe('sendFeedback', () => { it('throws when message is empty', () => { mockSdk(); - expect(() => sendFeedback({ message: '' })).toThrow('Unable to submit feedback with empty message'); + expectThrowsWithError(() => sendFeedback({ message: '' }), 'Unable to submit feedback with empty message'); }); it('throws when no client is set up', async () => { @@ -279,7 +291,7 @@ describe('sendFeedback', () => { getGlobalScope().setClient(undefined); getCurrentScope().setClient(undefined); getIsolationScope().setClient(undefined); - expect(() => sendFeedback({ message: 'mi' })).toThrow('No client setup, cannot send feedback.'); + expectThrowsWithError(() => sendFeedback({ message: 'mi' }), 'No client setup, cannot send feedback.'); }); it('uses provided errorMessages overrides', async () => { @@ -288,9 +300,10 @@ describe('sendFeedback', () => { return Promise.resolve({ statusCode: 403 }); }); - await expect( + await expectRejectsWithError( sendFeedback({ message: 'mi' }, { errorMessages: { ERROR_FORBIDDEN: 'custom forbidden text' } }), - ).rejects.toMatch('custom forbidden text'); + 'custom forbidden text', + ); }); it('falls back to default messages for codes not in errorMessages', async () => { @@ -300,9 +313,8 @@ describe('sendFeedback', () => { }); // Only override ERROR_FORBIDDEN — a 400 should still use the default generic message. - await expect( + await expectRejectsWithError( sendFeedback({ message: 'mi' }, { errorMessages: { ERROR_FORBIDDEN: 'custom forbidden text' } }), - ).rejects.toMatch( 'Unable to send feedback. This could be because of network issues, or because you are using an ad-blocker.', ); }); @@ -313,13 +325,12 @@ describe('sendFeedback', () => { return Promise.resolve({ statusCode: 400 }); }); - await expect( + await expectRejectsWithError( sendFeedback({ name: 'doe', email: 're@example.org', message: 'mi', }), - ).rejects.toMatch( 'Unable to send feedback. This could be because of network issues, or because you are using an ad-blocker.', ); }); @@ -330,13 +341,12 @@ describe('sendFeedback', () => { return Promise.resolve({ statusCode: 0 }); }); - await expect( + await expectRejectsWithError( sendFeedback({ name: 'doe', email: 're@example.org', message: 'mi', }), - ).rejects.toMatch( 'Unable to send feedback. This could be because of network issues, or because you are using an ad-blocker.', ); }); @@ -347,13 +357,12 @@ describe('sendFeedback', () => { return Promise.resolve({ statusCode: 403 }); }); - await expect( + await expectRejectsWithError( sendFeedback({ name: 'doe', email: 're@example.org', message: 'mi', }), - ).rejects.toMatch( 'Unable to send feedback. This could be because this domain is not in your list of allowed domains.', ); }); @@ -389,7 +398,7 @@ describe('sendFeedback', () => { vi.advanceTimersByTime(30_000); - await expect(promise).rejects.toMatch('Unable to determine if Feedback was correctly sent.'); + await expectRejectsWithError(promise, 'Unable to determine if Feedback was correctly sent.'); vi.useRealTimers(); });