Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
8 changes: 4 additions & 4 deletions packages/feedback/src/core/sendFeedback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) => {
Expand All @@ -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));
});
});
};
Expand Down
2 changes: 1 addition & 1 deletion packages/feedback/src/modal/components/Form.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/feedback/src/util/createFeedbackError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const DEFAULT_MESSAGES: Record<FeedbackErrorCode, string> = {
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];
}

Expand Down
35 changes: 22 additions & 13 deletions packages/feedback/test/core/sendFeedback.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<unknown>, message: string): Promise<void> {
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');
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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.',
);
});
Expand All @@ -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.',
);
});
Expand All @@ -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.',
);
});
Expand All @@ -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.',
);
});
Expand Down Expand Up @@ -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();
});
Expand Down
Loading