Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(2.1) Handles Capture feedback form network connection issues #4364

Open
wants to merge 17 commits into
base: antonis/3859-newCaptureFeedbackAPI-Form
Choose a base branch
from
Open

(2.1) Handles Capture feedback form network connection issues #4364

wants to merge 17 commits into from
+184 −14

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Dec 12, 2024 •
edited
Loading

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

Based on #4328

📜 Description

Checks the network connection before sending user feedback.

OK Error
Simulator Screenshot - iPhone SE (3rd generation) - 2024-12-12 at 16 16 37 Simulator Screenshot - iPhone SE (3rd generation) - 2024-12-12 at 16 12 37

Alternative approach

An alternative approach was initially tested that used sendFeedback function from the JS SDK that catches errors and returns related messages. The problem with that approach was that it always returned an error even when the call was successful. I think this might be due to a missing return statement on the promise resolve.

Given that the JS SDK seems to only check for network errors I considered the current simple approach good enough for now. I've opened a PR getsentry/sentry-javascript#14683 to fix this on the JS side (related issue) and we can iterate when the next JS SDK is released.

Related react native code patch 

diff --git a/packages/core/package.json b/packages/core/package.json
index dd88c414..189518c8 100644
--- a/packages/core/package.json
+++ b/packages/core/package.json
@@ -80,6 +80,7 @@
     "@react-native/babel-preset": "0.76.3",
     "@sentry-internal/eslint-config-sdk": "8.40.0",
     "@sentry-internal/eslint-plugin-sdk": "8.40.0",
+    "@sentry-internal/feedback": "8.40.0",
     "@sentry-internal/typescript": "8.40.0",
     "@sentry/wizard": "3.36.0",
     "@testing-library/react-native": "^12.7.2",
diff --git a/packages/core/src/js/feedback/FeedbackForm.tsx b/packages/core/src/js/feedback/FeedbackForm.tsx
index b79fe793..cc1e47ad 100644
--- a/packages/core/src/js/feedback/FeedbackForm.tsx
+++ b/packages/core/src/js/feedback/FeedbackForm.tsx
@@ -1,13 +1,22 @@
-import { captureFeedback, lastEventId } from '@sentry/core';
-import type { SendFeedbackParams } from '@sentry/types';
+import { lastEventId } from '@sentry/core';
+import type { EventHint, SendFeedbackParams } from '@sentry/types';
 import * as React from 'react';
 import type { KeyboardTypeOptions } from 'react-native';
 import { Alert, Text, TextInput, TouchableOpacity, View } from 'react-native';
-
+import { sendFeedback } from '@sentry-internal/feedback';
 import { defaultConfiguration } from './defaults';
 import defaultStyles from './FeedbackForm.styles';
 import type { FeedbackFormProps, FeedbackFormState, FeedbackFormStyles,FeedbackGeneralConfiguration, FeedbackTextConfiguration } from './FeedbackForm.types';
 
+const submitFeedback = async (feedbackParams: SendFeedbackParams, success: () => void, error: (e: string) => void): Promise<void> => {
+  try {
+    await sendFeedback(feedbackParams);
+    success();
+  } catch (e) {
+    error(e.toString());
+  }
+};
+
 /**
  * @beta
  * Implements a feedback form screen that sends feedback to Sentry using Sentry.captureFeedback.
@@ -53,11 +62,13 @@ export class FeedbackForm extends React.Component<FeedbackFormProps, FeedbackFor
       associatedEventId: eventId,
     };
 
-    onFormClose();
-    this.setState({ isVisible: false });
-
-    captureFeedback(userFeedback);
-    Alert.alert(text.successMessageText);
+    submitFeedback(userFeedback, () => {
+      onFormClose();
+      this.setState({ isVisible: false });
+      captureFeedback(userFeedback);
+      Alert.alert(text.successMessageText);
+    }, (error: string) => {
+      Alert.alert(text.errorTitle, error);
+    });
   };
 
   /**

💡 Motivation and Context

Fixes #4359

💚 How did you test it?

Manual testing, Unit tests

📝 Checklist

  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

#skip-changelog

@antonis antonis marked this pull request as ready for review December 12, 2024 15:16
@antonis antonis mentioned this pull request Dec 12, 2024
Open
10 tasks
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 12, 2024 •
edited
Loading

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 373.74 ms 412.16 ms 38.42 ms
Size 7.15 MiB 8.38 MiB 1.23 MiB

Baseline results on branch: antonis/3859-newCaptureFeedbackAPI-Form

Startup times

Revision Plain With Sentry Diff
a06f6ba+dirty 381.50 ms 429.77 ms 48.27 ms
1dd8d17+dirty 383.20 ms 432.62 ms 49.41 ms
d33790a+dirty 404.87 ms 473.06 ms 68.19 ms
a3ba405+dirty 359.67 ms 436.86 ms 77.19 ms
0781f75+dirty 406.72 ms 454.80 ms 48.08 ms
f4a5053+dirty 391.02 ms 427.04 ms 36.02 ms
03c9048+dirty 397.35 ms 417.73 ms 20.37 ms
50c70c0+dirty 385.30 ms 433.06 ms 47.76 ms
cadf235+dirty 455.51 ms 451.64 ms -3.87 ms
27e1bf3+dirty 398.69 ms 439.39 ms 40.69 ms

App size

Revision Plain With Sentry Diff
a06f6ba+dirty 7.15 MiB 8.37 MiB 1.22 MiB
1dd8d17+dirty 7.15 MiB 8.38 MiB 1.23 MiB
d33790a+dirty 7.15 MiB 8.38 MiB 1.23 MiB
a3ba405+dirty 7.15 MiB 8.37 MiB 1.22 MiB
0781f75+dirty 7.15 MiB 8.37 MiB 1.22 MiB
f4a5053+dirty 7.15 MiB 8.38 MiB 1.23 MiB
03c9048+dirty 7.15 MiB 8.38 MiB 1.23 MiB
50c70c0+dirty 7.15 MiB 8.38 MiB 1.23 MiB
cadf235+dirty 7.15 MiB 8.37 MiB 1.22 MiB
27e1bf3+dirty 7.15 MiB 8.37 MiB 1.22 MiB

Previous results on branch: antonis/4359-Feedback-Form-NetworkError

Startup times

Revision Plain With Sentry Diff
d232ce7+dirty 401.80 ms 423.51 ms 21.71 ms
274d5b5+dirty 413.06 ms 456.54 ms 43.48 ms
06ab320+dirty 382.32 ms 433.65 ms 51.33 ms
25507c5+dirty 378.89 ms 434.94 ms 56.05 ms
0131975+dirty 402.71 ms 454.00 ms 51.29 ms
5679aac+dirty 403.96 ms 470.57 ms 66.61 ms

App size

Revision Plain With Sentry Diff
d232ce7+dirty 7.15 MiB 8.37 MiB 1.22 MiB
274d5b5+dirty 7.15 MiB 8.37 MiB 1.22 MiB
06ab320+dirty 7.15 MiB 8.38 MiB 1.23 MiB
25507c5+dirty 7.15 MiB 8.38 MiB 1.23 MiB
0131975+dirty 7.15 MiB 8.38 MiB 1.23 MiB
5679aac+dirty 7.15 MiB 8.38 MiB 1.23 MiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 12, 2024 •
edited
Loading

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1225.55 ms 1232.18 ms 6.63 ms
Size 2.36 MiB 3.13 MiB 782.97 KiB

Baseline results on branch: antonis/3859-newCaptureFeedbackAPI-Form

Startup times

Revision Plain With Sentry Diff
a06f6ba+dirty 1230.45 ms 1227.09 ms -3.36 ms
1dd8d17+dirty 1235.22 ms 1218.96 ms -16.27 ms
0781f75+dirty 1222.19 ms 1222.11 ms -0.08 ms
cadf235+dirty 1223.89 ms 1236.22 ms 12.33 ms
38a278b+dirty 1233.02 ms 1233.82 ms 0.80 ms
a3ba405+dirty 1223.00 ms 1219.06 ms -3.94 ms
db1844e+dirty 1230.79 ms 1234.22 ms 3.43 ms
e0624b6+dirty 1221.86 ms 1226.42 ms 4.57 ms
26fc306+dirty 1227.25 ms 1225.85 ms -1.40 ms
50c70c0+dirty 1228.06 ms 1224.43 ms -3.64 ms

App size

Revision Plain With Sentry Diff
a06f6ba+dirty 2.36 MiB 3.11 MiB 761.35 KiB
1dd8d17+dirty 2.36 MiB 3.11 MiB 761.66 KiB
0781f75+dirty 2.36 MiB 3.11 MiB 761.35 KiB
cadf235+dirty 2.36 MiB 3.11 MiB 761.47 KiB
38a278b+dirty 2.36 MiB 3.13 MiB 782.12 KiB
a3ba405+dirty 2.36 MiB 3.11 MiB 760.99 KiB
db1844e+dirty 2.36 MiB 3.13 MiB 782.13 KiB
e0624b6+dirty 2.36 MiB 3.11 MiB 761.16 KiB
26fc306+dirty 2.36 MiB 3.11 MiB 761.18 KiB
50c70c0+dirty 2.36 MiB 3.11 MiB 760.92 KiB

Previous results on branch: antonis/4359-Feedback-Form-NetworkError

Startup times

Revision Plain With Sentry Diff
0131975+dirty 1231.33 ms 1227.10 ms -4.23 ms
274d5b5+dirty 1221.69 ms 1227.56 ms 5.87 ms
f4422db+dirty 1232.53 ms 1225.81 ms -6.72 ms
5679aac+dirty 1237.88 ms 1231.70 ms -6.18 ms
d232ce7+dirty 1223.47 ms 1226.79 ms 3.32 ms
25507c5+dirty 1230.96 ms 1229.94 ms -1.02 ms
06ab320+dirty 1222.00 ms 1220.50 ms -1.50 ms

App size

Revision Plain With Sentry Diff
0131975+dirty 2.36 MiB 3.11 MiB 761.34 KiB
274d5b5+dirty 2.36 MiB 3.11 MiB 761.55 KiB
f4422db+dirty 2.36 MiB 3.13 MiB 782.34 KiB
5679aac+dirty 2.36 MiB 3.11 MiB 761.93 KiB
d232ce7+dirty 2.36 MiB 3.11 MiB 761.50 KiB
25507c5+dirty 2.36 MiB 3.11 MiB 761.92 KiB
06ab320+dirty 2.36 MiB 3.11 MiB 761.14 KiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 12, 2024 •
edited
Loading

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1244.08 ms 1238.82 ms -5.26 ms
Size 2.92 MiB 3.69 MiB 794.10 KiB

Baseline results on branch: antonis/3859-newCaptureFeedbackAPI-Form

Startup times

Revision Plain With Sentry Diff
a06f6ba+dirty 1235.31 ms 1238.76 ms 3.45 ms
1dd8d17+dirty 1229.28 ms 1224.92 ms -4.36 ms
0781f75+dirty 1247.90 ms 1237.11 ms -10.79 ms
cadf235+dirty 1225.19 ms 1231.65 ms 6.47 ms
38a278b+dirty 1247.80 ms 1244.61 ms -3.18 ms
a3ba405+dirty 1229.31 ms 1228.16 ms -1.14 ms
db1844e+dirty 1231.94 ms 1238.86 ms 6.92 ms
e0624b6+dirty 1229.19 ms 1232.18 ms 3.00 ms
26fc306+dirty 1229.10 ms 1227.88 ms -1.22 ms
50c70c0+dirty 1226.61 ms 1225.02 ms -1.59 ms

App size

Revision Plain With Sentry Diff
a06f6ba+dirty 2.92 MiB 3.67 MiB 773.87 KiB
1dd8d17+dirty 2.92 MiB 3.67 MiB 774.21 KiB
0781f75+dirty 2.92 MiB 3.67 MiB 773.83 KiB
cadf235+dirty 2.92 MiB 3.67 MiB 773.97 KiB
38a278b+dirty 2.92 MiB 3.69 MiB 793.49 KiB
a3ba405+dirty 2.92 MiB 3.67 MiB 773.65 KiB
db1844e+dirty 2.92 MiB 3.69 MiB 793.48 KiB
e0624b6+dirty 2.92 MiB 3.67 MiB 773.62 KiB
26fc306+dirty 2.92 MiB 3.67 MiB 773.77 KiB
50c70c0+dirty 2.92 MiB 3.67 MiB 773.48 KiB

Previous results on branch: antonis/4359-Feedback-Form-NetworkError

Startup times

Revision Plain With Sentry Diff
0131975+dirty 1242.96 ms 1240.12 ms -2.83 ms
274d5b5+dirty 1235.24 ms 1234.41 ms -0.84 ms
f4422db+dirty 1233.30 ms 1231.47 ms -1.83 ms
5679aac+dirty 1242.61 ms 1234.55 ms -8.06 ms
d232ce7+dirty 1234.69 ms 1240.02 ms 5.33 ms
25507c5+dirty 1226.49 ms 1228.98 ms 2.49 ms
06ab320+dirty 1235.38 ms 1241.82 ms 6.43 ms

App size

Revision Plain With Sentry Diff
0131975+dirty 2.92 MiB 3.67 MiB 773.85 KiB
274d5b5+dirty 2.92 MiB 3.67 MiB 774.02 KiB
f4422db+dirty 2.92 MiB 3.69 MiB 793.65 KiB
5679aac+dirty 2.92 MiB 3.67 MiB 774.41 KiB
d232ce7+dirty 2.92 MiB 3.67 MiB 773.91 KiB
25507c5+dirty 2.92 MiB 3.67 MiB 774.58 KiB
06ab320+dirty 2.92 MiB 3.67 MiB 773.58 KiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 12, 2024 •
edited
Loading

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 475.17 ms 480.38 ms 5.21 ms
Size 17.74 MiB 20.10 MiB 2.37 MiB

Baseline results on branch: antonis/3859-newCaptureFeedbackAPI-Form

Startup times

Revision Plain With Sentry Diff
03c9048 500.96 ms 486.65 ms -14.31 ms
561640f 461.96 ms 458.11 ms -3.85 ms
a3ba405 438.16 ms 435.78 ms -2.38 ms
50c70c0 496.82 ms 526.02 ms 29.20 ms
cadf235 462.20 ms 463.34 ms 1.14 ms
f4a5053 478.22 ms 458.35 ms -19.87 ms
1dd8d17 399.65 ms 393.81 ms -5.84 ms
a06f6ba 424.02 ms 415.82 ms -8.20 ms
0781f75 452.32 ms 457.22 ms 4.91 ms
e0624b6 447.67 ms 441.08 ms -6.59 ms

App size

Revision Plain With Sentry Diff
03c9048 17.74 MiB 20.10 MiB 2.37 MiB
561640f 17.74 MiB 20.09 MiB 2.35 MiB
a3ba405 17.74 MiB 20.09 MiB 2.35 MiB
50c70c0 17.74 MiB 20.10 MiB 2.36 MiB
cadf235 17.74 MiB 20.09 MiB 2.35 MiB
f4a5053 17.74 MiB 20.10 MiB 2.36 MiB
1dd8d17 17.74 MiB 20.10 MiB 2.36 MiB
a06f6ba 17.74 MiB 20.09 MiB 2.35 MiB
0781f75 17.74 MiB 20.09 MiB 2.35 MiB
e0624b6 17.74 MiB 20.10 MiB 2.36 MiB

Previous results on branch: antonis/4359-Feedback-Form-NetworkError

Startup times

Revision Plain With Sentry Diff
0131975 436.28 ms 427.60 ms -8.68 ms
274d5b5 436.44 ms 436.02 ms -0.42 ms
25507c5 448.50 ms 439.85 ms -8.65 ms
5679aac 452.07 ms 470.16 ms 18.09 ms
06ab320 438.67 ms 425.40 ms -13.27 ms
d232ce7 434.55 ms 442.12 ms 7.57 ms

App size

Revision Plain With Sentry Diff
0131975 17.74 MiB 20.10 MiB 2.36 MiB
274d5b5 17.74 MiB 20.09 MiB 2.35 MiB
25507c5 17.74 MiB 20.10 MiB 2.37 MiB
5679aac 17.74 MiB 20.10 MiB 2.37 MiB
06ab320 17.74 MiB 20.10 MiB 2.36 MiB
d232ce7 17.74 MiB 20.09 MiB 2.35 MiB

lucas-zimerman
lucas-zimerman reviewed Dec 12, 2024
View reviewed changes
packages/core/src/js/feedback/utils.ts Outdated
onDisconnected();
}
} catch (error) {
onDisconnected();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i: We should add an onError call and pass error as a parameter, so users know they are facing an issue that isn't unrelated to connectivity issues (for example, wrong dsn, server rejecting the feedback, some sdk error,...)

a suggestion for the error message: "Unable to send feedback due to an unexpected error."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea 👍
Added an onError method to communicate the unexpected error with 40b4fd3

@antonis antonis linked an issue Dec 13, 2024 that may be closed by this pull request
Handle feedback form submision errors #4359
Open
@krystofwoldrich krystofwoldrich changed the title Handles Capture feedback form network connection issues (2.1) Handles Capture feedback form network connection issues Dec 13, 2024
@antonis
Merge branch 'antonis/3859-newCaptureFeedbackAPI-Form' into antonis/4…
4811f36 
…359-Feedback-Form-NetworkError

# Conflicts:
#	packages/core/src/js/feedback/FeedbackForm.tsx
@antonis
Copy link
Collaborator Author

antonis commented Dec 17, 2024

I plan to iterate with the alternative approach since the latest JS SDK includes the SendFeedback fix

@antonis antonis mentioned this pull request Dec 17, 2024
Draft
10 tasks
@antonis
Merge branch 'antonis/3859-newCaptureFeedbackAPI-Form' into antonis/4…
e4e23a6 
…359-Feedback-Form-NetworkError

# Conflicts:
#	packages/core/src/js/feedback/FeedbackForm.tsx
#	packages/core/test/feedback/FeedbackForm.test.tsx
@lucas-zimerman
Copy link
Collaborator

I plan to iterate with the alternative approach sinc

So this PR is blocked by #4383 ?

@antonis
Copy link
Collaborator Author

antonis commented Dec 18, 2024 •
edited
Loading

I plan to iterate with the alternative approach sinc

So this PR is blocked by #4383 ?

I think we can start with the current approach and then iterate with an improvement with #4383

@antonis
Merge branch 'antonis/3859-newCaptureFeedbackAPI-Form' into antonis/4…
d84ae63 
…359-Feedback-Form-NetworkError
@antonis
Merge branch 'antonis/3859-newCaptureFeedbackAPI-Form' into antonis/4…
1192b17 
…359-Feedback-Form-NetworkError

# Conflicts:
#	packages/core/src/js/feedback/FeedbackForm.types.ts
@antonis
Add onFormSubmitted dev note
6ffae7d
@antonis
Merge branch 'antonis/3859-newCaptureFeedbackAPI-Form' into antonis/4…
d25b188 
…359-Feedback-Form-NetworkError
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucas-zimerman lucas-zimerman lucas-zimerman left review comments

@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich krystofwoldrich is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle feedback form submision errors
2 participants
@antonis @lucas-zimerman