Skip to content

Commit 1272fb6

Browse files
logaretmclaude
andcommitted
address PR review feedback
- Form: use ?? so empty-string text overrides are respected (Copilot) - constants: grammar fix "with an empty message" (Copilot) - sendFeedback: clean up afterSendEvent listener on timeout to avoid leak (Copilot) - tests: cover empty-message and no-client early-return paths (Copilot) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 70d2ab4 commit 1272fb6

4 files changed

Lines changed: 21 additions & 3 deletions

File tree

packages/feedback/src/constants/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ export const HIGHLIGHT_TOOL_TEXT = 'Highlight';
2626
export const HIDE_TOOL_TEXT = 'Hide';
2727
export const REMOVE_HIGHLIGHT_TEXT = 'Remove';
2828

29-
export const ERROR_EMPTY_MESSAGE_TEXT = 'Unable to submit feedback with empty message';
29+
export const ERROR_EMPTY_MESSAGE_TEXT = 'Unable to submit feedback with an empty message';
3030
export const ERROR_NO_CLIENT_TEXT = 'No client setup, cannot send feedback.';
3131
export const ERROR_TIMEOUT_TEXT = 'Unable to determine if Feedback was correctly sent.';
3232
export const ERROR_FORBIDDEN_TEXT =

packages/feedback/src/core/sendFeedback.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,10 @@ export const sendFeedback: SendFeedback = (
3737
// We want to wait for the feedback to be sent (or not)
3838
return new Promise<string>((resolve, reject) => {
3939
// After 30s, we want to clear anyhow
40-
const timeout = setTimeout(() => reject('ERROR_TIMEOUT' satisfies FeedbackErrorCode), 30_000);
40+
const timeout = setTimeout(() => {
41+
cleanup();
42+
reject('ERROR_TIMEOUT' satisfies FeedbackErrorCode);
43+
}, 30_000);
4144

4245
const cleanup = client.on('afterSendEvent', (event: Event, response: TransportMakeRequestResponse) => {
4346
if (event.event_id !== eventId) {

packages/feedback/src/modal/components/Form.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ export function Form({
146146
} catch (error) {
147147
DEBUG_BUILD && debug.error(error);
148148
const err = error instanceof Error ? error : new Error(String(error));
149-
setError(errorTextByCode[err.message as FeedbackErrorCode] || errorGenericText);
149+
setError(errorTextByCode[err.message as FeedbackErrorCode] ?? errorGenericText);
150150
onSubmitError(err);
151151
}
152152
} finally {

packages/feedback/test/core/sendFeedback.test.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,21 @@ describe('sendFeedback', () => {
267267
]);
268268
});
269269

270+
it('throws when message is empty', () => {
271+
mockSdk();
272+
expect(() => sendFeedback({ message: '' })).toThrow('ERROR_EMPTY_MESSAGE');
273+
});
274+
275+
it('throws when no client is set up', async () => {
276+
// Isolate in its own scope so the client set up by other tests doesn't bleed in.
277+
// `getClient` reads from the current scope; resetting it here leaves no client.
278+
const { getGlobalScope } = await import('@sentry/core');
279+
getGlobalScope().setClient(undefined);
280+
getCurrentScope().setClient(undefined);
281+
getIsolationScope().setClient(undefined);
282+
expect(() => sendFeedback({ message: 'mi' })).toThrow('ERROR_NO_CLIENT');
283+
});
284+
270285
it('handles 400 transport error', async () => {
271286
mockSdk();
272287
vi.spyOn(getClient()!.getTransport()!, 'send').mockImplementation(() => {

0 commit comments

Comments
 (0)