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, }