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 captureFeedback errors #4364

Merged
merged 115 commits into from
Jan 15, 2025
Merged

(2.1) Handles captureFeedback errors #4364

merged 115 commits into from
Jan 15, 2025

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

  • Handles captureFeedback errors
  • Aligns with the JS callbacks API by adding the onFormOpen, onSubmitSuccess, onSubmitError, onFormSubmitted

💡 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 and others added 30 commits November 27, 2024 14:00
@antonis
Update the client implementation to use the new capture feedback js api
817eac8
@antonis
Updates SDK API
5370a99
@antonis
Adds new feedback button in the sample
42e2fa1
@antonis
Adds changelog
514b102
@antonis
Removes unused mock
0935bbd
@antonis @krystofwoldrich
Update CHANGELOG.md
9ea5496 
Co-authored-by: Krystof Woldrich <[email protected]>
@antonis
Directly use captureFeedback from sentry/core
da0d4ac
@antonis
Use import from core
3e36c6d
@antonis
Fixes imports order lint issue
5f3df64
@antonis
Fixes build issue
71b28e8
@antonis
Adds captureFeedback tests from sentry-javascript
f9d2b59
@krystofwoldrich
Update CHANGELOG.md
d05d531
@antonis
Only deprecate client captureUserFeedback
2bb104b
@antonis
Add simple form UI
4339274
@antonis
Adds basic form functionality
6ce799b
@antonis
Update imports
f2cefc6
@antonis
Update imports
694ee33
@antonis
Merge branch 'main' into antonis/3859-newCaptureFeedbackAPI-Form
18b1c33
@antonis
Remove useState hook to avoid multiple react instances issues
034efde
@antonis
Move types and styles in different files
67a492d
@antonis
Removes attachment button to be added back separately along with the …
8eaa61d 
…implementation
@antonis
Add basic field validation
0b88cc5
@antonis
Adds changelog
ae11b8d
@antonis
Updates changelog
7f2ca06
@antonis
Updates changelog
064b6c4
@antonis
Merge branch 'main' into antonis/3859-newCaptureFeedbackAPI-Form
e2add4a
@antonis
Trim whitespaces from the submitted feedback
e21718a
@antonis
Adds tests
407f179
@antonis
Merge branch 'main' into antonis/3859-newCaptureFeedbackAPI-Form
14ac005
@antonis
Renames FeedbackFormScreen to FeedbackForm
ddade00
antonis added 10 commits December 19, 2024 10:51
@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
Align changelog entry
5f9dec6
@antonis
Merge branch 'main' into antonis/3859-newCaptureFeedbackAPI-Form
8359696
@antonis
Add onFormSubmitted dev note
6ffae7d
@antonis
Merge branch 'main' into antonis/3859-newCaptureFeedbackAPI-Form
76518cc
@antonis
Merge branch 'antonis/3859-newCaptureFeedbackAPI-Form' into antonis/4…
d25b188 
…359-Feedback-Form-NetworkError
@antonis
Merge branch 'main' into antonis/3859-newCaptureFeedbackAPI-Form
b0384ed
@antonis
Merge branch 'main' into antonis/3859-newCaptureFeedbackAPI-Form
61886c2
@antonis
Update changelog
9853630
@antonis
Merge branch 'antonis/3859-newCaptureFeedbackAPI-Form' into antonis/4…
5adcea3 
…359-Feedback-Form-NetworkError
Base automatically changed from antonis/3859-newCaptureFeedbackAPI-Form to feedback-ui January 10, 2025 10:32
@antonis antonis mentioned this pull request Jan 10, 2025
Draft
10 tasks
krystofwoldrich
krystofwoldrich requested changes Jan 10, 2025
View reviewed changes
Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

The transport in mobile SDK works differently than the JS transport, and we currently don't have the response available, so the sendFeedback will still throw event after the fix in JS.

See impl of sendEnvelope in RN

sentry-react-native/packages/core/src/js/client.ts

Line 132 in 18ce5e8

return Promise.resolve({});

I think we should avoid checking the connection before send, because the native transport handles that (it stores the envelope offline and tries to send it later).

We can just try catch our current captureFeedback api for the on error call back. And note in the docs that if the device is offline the envelope is sent later.

@antonis antonis changed the title (2.1) Handles Capture feedback form network connection issues (2.1) Handles captureFeedback errors Jan 10, 2025
@antonis
Copy link
Collaborator Author

antonis commented Jan 10, 2025

Thank you for the feedback @krystofwoldrich and the explanation on why not to use sendFeedback 🙇

We can just try catch our current captureFeedback api for the on error call back. And note in the docs that if the device is offline the envelope is sent later.

Makes sense 👍 Updated with c014ccc

@antonis antonis mentioned this pull request Jan 10, 2025
Open
lucas-zimerman
lucas-zimerman approved these changes Jan 13, 2025
View reviewed changes
Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you for the PR!

krystofwoldrich
krystofwoldrich approved these changes Jan 15, 2025
View reviewed changes
Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

Thank you for all the changes and refs.

Looks good! 🚀

@krystofwoldrich krystofwoldrich merged commit 4529b68 into feedback-ui Jan 15, 2025
66 checks passed
@krystofwoldrich krystofwoldrich deleted the antonis/4359-Feedback-Form-NetworkError branch January 15, 2025 17:44
@antonis antonis mentioned this pull request Jan 17, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krystofwoldrich krystofwoldrich krystofwoldrich approved these changes

@lucas-zimerman lucas-zimerman lucas-zimerman approved these changes

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
3 participants
@antonis @lucas-zimerman @krystofwoldrich