From cd3317627232b68f4ade37af05a00a8c336a66bf Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Wed, 23 Sep 2026 10:13:54 +0200 Subject: [PATCH] fix(core): Prevent duplicate feedback submissions and keep draft on error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FeedbackForm called the fire-and-forget captureFeedback and immediately reported success, cleared the saved draft, and had no in-flight guard, so repeated taps could submit multiple times. The React Native client hands the envelope to the (native or JS) transport without surfacing the delivery result, so the JS layer can't confirm the feedback reached Sentry — success is therefore reported optimistically once the event is captured. What this fixes: - A synchronous in-flight guard prevents duplicate submissions (including repeated taps and taps after a successful submit when a custom onFormSubmitted keeps the form mounted). - When feedback can't be captured (no active client, or captureFeedback throws) the form now reports an error via onSubmitError and keeps the draft intact so the user can retry, instead of falsely reporting success. - The submit button is disabled while submitting. Fixes #6766 Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 1 + packages/core/etc/sentry-react-native.api.md | 2 + .../src/js/feedback/FeedbackForm.styles.ts | 3 + .../core/src/js/feedback/FeedbackForm.tsx | 104 +++++++++++--- .../src/js/feedback/FeedbackForm.types.ts | 6 + .../core/test/feedback/FeedbackForm.test.tsx | 127 ++++++++++++++++++ .../__snapshots__/FeedbackForm.test.tsx.snap | 12 +- .../FeedbackFormManager.test.tsx.snap | 14 +- 8 files changed, 241 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2c0ae7794..0ed15bfb4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ - Android Gradle plugin no longer writes generated `sentry.options.json` into your source tree during builds ([#6751](https://github.com/getsentry/sentry-react-native/pull/6751)) - Android Gradle plugin no longer writes generated `modules.json` into your source tree during release builds ([#6753](https://github.com/getsentry/sentry-react-native/pull/6753)) - Fix Mac Catalyst linking the wrong `Sentry.xcframework` slice ([#6758](https://github.com/getsentry/sentry-react-native/pull/6758)) +- `FeedbackForm` prevents duplicate submissions and keeps the draft on error ([#6769](https://github.com/getsentry/sentry-react-native/pull/6769)) ### Internal diff --git a/packages/core/etc/sentry-react-native.api.md b/packages/core/etc/sentry-react-native.api.md index d19ba75f15..d0dde858ba 100644 --- a/packages/core/etc/sentry-react-native.api.md +++ b/packages/core/etc/sentry-react-native.api.md @@ -471,6 +471,8 @@ export interface FeedbackFormStyles { // (undocumented) submitButton?: ViewStyle; // (undocumented) + submitButtonDisabled?: ViewStyle; + // (undocumented) submitText?: TextStyle; // (undocumented) takeScreenshotButton?: ViewStyle; diff --git a/packages/core/src/js/feedback/FeedbackForm.styles.ts b/packages/core/src/js/feedback/FeedbackForm.styles.ts index 8a10494895..979ef10ad7 100644 --- a/packages/core/src/js/feedback/FeedbackForm.styles.ts +++ b/packages/core/src/js/feedback/FeedbackForm.styles.ts @@ -84,6 +84,9 @@ const defaultStyles = (theme: FeedbackFormTheme): FeedbackFormStyles => { alignItems: 'center', marginBottom: 10, }, + submitButtonDisabled: { + opacity: 0.7, + }, submitText: { color: theme.accentForeground, fontSize: 18, diff --git a/packages/core/src/js/feedback/FeedbackForm.tsx b/packages/core/src/js/feedback/FeedbackForm.tsx index 1b221c7f81..a7e2c2a23a 100644 --- a/packages/core/src/js/feedback/FeedbackForm.tsx +++ b/packages/core/src/js/feedback/FeedbackForm.tsx @@ -2,7 +2,15 @@ import type { SendFeedbackParams, User } from '@sentry/core'; import type { KeyboardTypeOptions, NativeEventSubscription } from 'react-native'; -import { captureFeedback, debug, getCurrentScope, getGlobalScope, getIsolationScope, lastEventId } from '@sentry/core'; +import { + captureFeedback, + debug, + getClient, + getCurrentScope, + getGlobalScope, + getIsolationScope, + lastEventId, +} from '@sentry/core'; import * as React from 'react'; import { Appearance, @@ -43,7 +51,7 @@ import { base64ToUint8Array, feedbackAlertDialog, isValidEmail } from './utils'; export class FeedbackForm extends React.Component { public static defaultProps = defaultConfiguration; - private static _savedState: Omit = { + private static _savedState: Omit = { name: '', email: '', description: '', @@ -56,6 +64,11 @@ export class FeedbackForm extends React.Component void = () => { const { name, email, description } = this.state; - const { onSubmitSuccess, onSubmitError, onFormSubmitted } = this.props; const text = this.props; + // Ignore repeated taps: while a submission is running, and after a successful submit + // (a custom `onFormSubmitted` may keep the form mounted). Uses a synchronous flag so + // two taps in the same tick can't both get through. + if (this._isSubmitting) { + return; + } + const trimmedName = name?.trim(); const trimmedEmail = email?.trim(); const trimmedDescription = description?.trim(); @@ -138,25 +158,75 @@ export class FeedbackForm extends React.Component | undefined, + ): void => { + const { onSubmitSuccess, onSubmitError, onFormSubmitted } = this.props; + const text = this.props; + try { + if (!getClient()) { + throw new Error('No Sentry client is available to send the feedback.'); + } + if (!onFormSubmitted) { this.setState({ isVisible: false }); } + captureFeedback(userFeedback, attachments ? { attachments } : undefined); - onSubmitSuccess({ - name: trimmedName, - email: trimmedEmail, - message: trimmedDescription, - attachments: attachments, - }); - feedbackAlertDialog(text.successMessageText, ''); - onFormSubmitted(); - this._didSubmitForm = true; } catch (error) { const errorString = `Feedback form submission failed: ${error}`; - onSubmitError(new Error(errorString)); + debug.error(errorString); + // Release the guard first so the user can retry even if `onSubmitError` throws. + this._isSubmitting = false; + this.setState({ isSubmitting: false }); + this._runCallback(() => onSubmitError(new Error(errorString))); feedbackAlertDialog(text.errorTitle, text.genericError); - debug.error(`Feedback form submission failed: ${error}`); + return; + } + + // The feedback was captured. Mark the form as submitted and keep `_isSubmitting` set so it + // can't be submitted again (even if a custom `onFormSubmitted` keeps it mounted). Each consumer + // callback is isolated so a throw in one can't undo the submission or skip the remaining success + // side-effects (the alert and closing the form). + this._didSubmitForm = true; + this._runCallback(() => + onSubmitSuccess({ + name: userFeedback.name ?? '', + email: userFeedback.email ?? '', + message: userFeedback.message, + attachments: attachments, + }), + ); + feedbackAlertDialog(text.successMessageText, ''); + this._runCallback(() => onFormSubmitted()); + }; + + /** + * Runs a consumer-provided callback, isolating any thrown error so it can't disrupt the + * submission flow. The feedback has already been captured by the time these run. + */ + private _runCallback = (callback: () => void): void => { + try { + callback(); + } catch (error) { + debug.error(`Feedback form callback threw: ${error}`); } }; @@ -402,7 +472,11 @@ export class FeedbackForm extends React.Component )} - + {text.submitButtonLabel} diff --git a/packages/core/src/js/feedback/FeedbackForm.types.ts b/packages/core/src/js/feedback/FeedbackForm.types.ts index 0a19663643..a7faae8c37 100644 --- a/packages/core/src/js/feedback/FeedbackForm.types.ts +++ b/packages/core/src/js/feedback/FeedbackForm.types.ts @@ -293,6 +293,7 @@ export interface FeedbackFormStyles { input?: TextStyle; textArea?: TextStyle; submitButton?: ViewStyle; + submitButtonDisabled?: ViewStyle; submitText?: TextStyle; cancelButton?: ViewStyle; cancelText?: TextStyle; @@ -343,6 +344,11 @@ export interface ScreenshotButtonStyles { */ export interface FeedbackFormState { isVisible: boolean; + /** + * Whether a submission is currently in flight (waiting for confirmation that the + * feedback was sent). Used to disable the submit button and prevent double submits. + */ + isSubmitting: boolean; name: string; email: string; description: string; diff --git a/packages/core/test/feedback/FeedbackForm.test.tsx b/packages/core/test/feedback/FeedbackForm.test.tsx index ddd6d195a5..8cbf7ceaf3 100644 --- a/packages/core/test/feedback/FeedbackForm.test.tsx +++ b/packages/core/test/feedback/FeedbackForm.test.tsx @@ -112,6 +112,10 @@ describe('FeedbackForm', () => { beforeEach(() => { mockIsolationScopeGetUser.mockReturnValue(undefined); mockGlobalScopeGetUser.mockReturnValue(undefined); + // An active client is required for a submission to be reported as successful. + const client = new TestClient(getDefaultTestClientOptions()); + setCurrentClient(client); + client.init(); FeedbackForm.reset(); }); @@ -425,6 +429,125 @@ describe('FeedbackForm', () => { }); }); + it('reports an error and keeps the draft when there is no active Sentry client', async () => { + // @ts-expect-error - simulate the SDK not being initialized. + setCurrentClient(undefined); + + const { getByPlaceholderText, getByText, unmount } = render(); + + fireEvent.changeText(getByPlaceholderText(defaultProps.namePlaceholder), 'John Doe'); + fireEvent.changeText(getByPlaceholderText(defaultProps.emailPlaceholder), 'john.doe@example.com'); + fireEvent.changeText(getByPlaceholderText(defaultProps.messagePlaceholder), 'This is a feedback message.'); + + fireEvent.press(getByText(defaultProps.submitButtonLabel)); + + await waitFor(() => { + expect(mockOnSubmitError).toHaveBeenCalled(); + expect(Alert.alert).toHaveBeenCalledWith(defaultProps.errorTitle, defaultProps.genericError); + }); + expect(mockOnSubmitSuccess).not.toHaveBeenCalled(); + + // The draft is preserved so the user can retry. + unmount(); + const { queryByPlaceholderText } = render(); + expect(queryByPlaceholderText(defaultProps.namePlaceholder).props.value).toBe('John Doe'); + expect(queryByPlaceholderText(defaultProps.messagePlaceholder).props.value).toBe('This is a feedback message.'); + }); + + it('does not submit again after a successful submission', async () => { + const { getByPlaceholderText, getByText } = render(); + + fireEvent.changeText(getByPlaceholderText(defaultProps.namePlaceholder), 'John Doe'); + fireEvent.changeText(getByPlaceholderText(defaultProps.emailPlaceholder), 'john.doe@example.com'); + fireEvent.changeText(getByPlaceholderText(defaultProps.messagePlaceholder), 'This is a feedback message.'); + + fireEvent.press(getByText(defaultProps.submitButtonLabel)); + // Repeated taps must not trigger additional submissions. + fireEvent.press(getByText(defaultProps.submitButtonLabel)); + + expect(captureFeedback).toHaveBeenCalledTimes(1); + expect(mockOnSubmitSuccess).toHaveBeenCalledTimes(1); + }); + + it('still shows success, closes the form, and blocks resubmit when onSubmitSuccess throws', async () => { + const throwingOnSubmitSuccess = jest.fn(() => { + throw new Error('callback error'); + }); + const { getByPlaceholderText, getByText } = render( + , + ); + + fireEvent.changeText(getByPlaceholderText(defaultProps.namePlaceholder), 'John Doe'); + fireEvent.changeText(getByPlaceholderText(defaultProps.emailPlaceholder), 'john.doe@example.com'); + fireEvent.changeText(getByPlaceholderText(defaultProps.messagePlaceholder), 'This is a feedback message.'); + + // The feedback is captured, then the success callback throws. + fireEvent.press(getByText(defaultProps.submitButtonLabel)); + // A second tap must not capture again: the feedback was already submitted. + fireEvent.press(getByText(defaultProps.submitButtonLabel)); + + expect(captureFeedback).toHaveBeenCalledTimes(1); + expect(throwingOnSubmitSuccess).toHaveBeenCalledTimes(1); + // A throwing success callback must not be treated as a submission failure... + expect(mockOnSubmitError).not.toHaveBeenCalled(); + // ...and must not skip the success alert or closing the form. + expect(Alert.alert).toHaveBeenCalledWith(defaultProps.successMessageText, ''); + expect(mockOnFormSubmitted).toHaveBeenCalled(); + }); + + it('allows retry when onSubmitError throws on a failed submission', async () => { + (captureFeedback as jest.Mock).mockImplementationOnce(() => { + throw new Error('capture error'); + }); + const throwingOnSubmitError = jest.fn(() => { + throw new Error('callback error'); + }); + + const { getByPlaceholderText, getByText } = render( + , + ); + + fireEvent.changeText(getByPlaceholderText(defaultProps.namePlaceholder), 'John Doe'); + fireEvent.changeText(getByPlaceholderText(defaultProps.emailPlaceholder), 'john.doe@example.com'); + fireEvent.changeText(getByPlaceholderText(defaultProps.messagePlaceholder), 'This is a feedback message.'); + + // First submission fails and its onSubmitError throws — the guard must still be released. + fireEvent.press(getByText(defaultProps.submitButtonLabel)); + expect(throwingOnSubmitError).toHaveBeenCalledTimes(1); + + // Second submission goes through. + fireEvent.press(getByText(defaultProps.submitButtonLabel)); + await waitFor(() => { + expect(mockOnSubmitSuccess).toHaveBeenCalled(); + }); + expect(captureFeedback).toHaveBeenCalledTimes(2); + }); + + it('allows re-submitting after a failed submission', async () => { + (captureFeedback as jest.Mock).mockImplementationOnce(() => { + throw new Error('Test error'); + }); + + const { getByPlaceholderText, getByText } = render(); + + fireEvent.changeText(getByPlaceholderText(defaultProps.namePlaceholder), 'John Doe'); + fireEvent.changeText(getByPlaceholderText(defaultProps.emailPlaceholder), 'john.doe@example.com'); + fireEvent.changeText(getByPlaceholderText(defaultProps.messagePlaceholder), 'This is a feedback message.'); + + // First submission fails and releases the in-flight guard. + fireEvent.press(getByText(defaultProps.submitButtonLabel)); + await waitFor(() => { + expect(mockOnSubmitError).toHaveBeenCalled(); + }); + + // Second submission goes through. + fireEvent.press(getByText(defaultProps.submitButtonLabel)); + await waitFor(() => { + expect(mockOnSubmitSuccess).toHaveBeenCalled(); + }); + expect(captureFeedback).toHaveBeenCalledTimes(2); + }); + it('calls onAddScreenshot when the screenshot button is pressed and no image picker library is integrated', async () => { const { getByText } = render(); @@ -516,6 +639,10 @@ describe('FeedbackForm', () => { fireEvent.changeText(getByPlaceholderText(defaultProps.messagePlaceholder), 'This is a feedback message.'); fireEvent.press(getByText(defaultProps.submitButtonLabel)); + await waitFor(() => { + expect(mockOnSubmitSuccess).toHaveBeenCalled(); + }); + unmount(); const { queryByPlaceholderText } = render(); diff --git a/packages/core/test/feedback/__snapshots__/FeedbackForm.test.tsx.snap b/packages/core/test/feedback/__snapshots__/FeedbackForm.test.tsx.snap index 5e4c549e07..597c452fe4 100644 --- a/packages/core/test/feedback/__snapshots__/FeedbackForm.test.tsx.snap +++ b/packages/core/test/feedback/__snapshots__/FeedbackForm.test.tsx.snap @@ -177,7 +177,7 @@ exports[`FeedbackForm matches the snapshot with custom styles 1`] = ` { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, } @@ -518,7 +518,7 @@ exports[`FeedbackForm matches the snapshot with custom styles and screenshot but { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, } @@ -796,7 +796,7 @@ exports[`FeedbackForm matches the snapshot with custom texts 1`] = ` { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, } @@ -1136,7 +1136,7 @@ exports[`FeedbackForm matches the snapshot with custom texts and screenshot butt { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, } @@ -1413,7 +1413,7 @@ exports[`FeedbackForm matches the snapshot with default configuration 1`] = ` { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, } @@ -1753,7 +1753,7 @@ exports[`FeedbackForm matches the snapshot with default configuration and screen { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, } diff --git a/packages/core/test/feedback/__snapshots__/FeedbackFormManager.test.tsx.snap b/packages/core/test/feedback/__snapshots__/FeedbackFormManager.test.tsx.snap index 04ea407406..3f8780fdf9 100644 --- a/packages/core/test/feedback/__snapshots__/FeedbackFormManager.test.tsx.snap +++ b/packages/core/test/feedback/__snapshots__/FeedbackFormManager.test.tsx.snap @@ -883,7 +883,7 @@ exports[`FeedbackButtonManager the Feedback Form matches the snapshot with custo { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, } @@ -1242,7 +1242,7 @@ exports[`FeedbackButtonManager the Feedback Form matches the snapshot with custo { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, } @@ -1601,7 +1601,7 @@ exports[`FeedbackButtonManager the Feedback Form matches the snapshot with defau { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, } @@ -1960,7 +1960,7 @@ exports[`FeedbackButtonManager the Feedback Form matches the snapshot with defau { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, } @@ -2319,7 +2319,7 @@ exports[`FeedbackButtonManager the Feedback Form matches the snapshot with defau { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, } @@ -2678,7 +2678,7 @@ exports[`FeedbackButtonManager the Feedback Form matches the snapshot with syste { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, } @@ -3037,7 +3037,7 @@ exports[`FeedbackButtonManager the Feedback Form matches the snapshot with syste { "busy": undefined, "checked": undefined, - "disabled": undefined, + "disabled": false, "expanded": undefined, "selected": undefined, }