Skip to content

SentryFeedbackWidget Improvements #2964

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.

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

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented May 27, 2025

📜 Description

  • pick screenshot
  • take screenshot
  • feedback options
Widget Take Remove
a c d

💡 Motivation and Context

Closes #2697

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • 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
  • All tests passing
  • No breaking changes

🔮 Next steps

  • Document setting of navigator key
  • Document iOS/Android strings from camera roll access not needed with current setup
  • Double check Android issue with app being terminated while picking image.

Copy link
Contributor

github-actions bot commented May 27, 2025

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 680f402

Copy link

codecov bot commented May 27, 2025

Codecov Report

Attention: Patch coverage is 97.94239% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.03%. Comparing base (bb57a41) to head (680f402).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...utter/lib/src/feedback/sentry_feedback_widget.dart 96.12% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2964      +/-   ##
==========================================
+ Coverage   87.72%   88.03%   +0.31%     
==========================================
  Files         286      287       +1     
  Lines        9335     9528     +193     
==========================================
+ Hits         8189     8388     +199     
+ Misses       1146     1140       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

github-actions bot commented May 27, 2025

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 548.33 ms 625.57 ms 77.24 ms
Size 6.54 MiB 7.53 MiB 1016.51 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8ced2dc 295.58 ms 336.49 ms 40.91 ms
4b29d6e 386.80 ms 430.86 ms 44.06 ms
5f443de 412.30 ms 491.67 ms 79.37 ms
04db237 330.16 ms 428.38 ms 98.22 ms
689d2fd 378.62 ms 430.48 ms 51.86 ms
6572f8d 302.35 ms 348.10 ms 45.75 ms
ccc09e4 308.21 ms 357.74 ms 49.54 ms
7954fb3 459.38 ms 500.72 ms 41.34 ms
33f99c9 668.33 ms 735.08 ms 66.76 ms
dd5521e 454.51 ms 538.29 ms 83.78 ms

App size

Revision Plain With Sentry Diff
8ced2dc 6.06 MiB 7.03 MiB 990.29 KiB
4b29d6e 6.33 MiB 7.26 MiB 946.14 KiB
5f443de 6.35 MiB 7.34 MiB 1008.00 KiB
04db237 5.94 MiB 6.95 MiB 1.01 MiB
689d2fd 6.06 MiB 7.10 MiB 1.04 MiB
6572f8d 6.15 MiB 7.13 MiB 999.97 KiB
ccc09e4 5.94 MiB 6.95 MiB 1.01 MiB
7954fb3 6.49 MiB 7.57 MiB 1.08 MiB
33f99c9 6.54 MiB 7.53 MiB 1017.45 KiB
dd5521e 6.44 MiB 7.50 MiB 1.06 MiB

Previous results on branch: feat/imporve-user-feedback-widget

Startup times

Revision Plain With Sentry Diff
d5bfab8 450.79 ms 492.04 ms 41.25 ms
f84f616 470.73 ms 503.86 ms 33.13 ms
5848af1 426.16 ms 489.23 ms 63.07 ms
d99d55b 458.37 ms 524.06 ms 65.69 ms
2a151e2 492.16 ms 564.78 ms 72.62 ms
23ff8a6 493.56 ms 556.38 ms 62.82 ms
62f0687 457.10 ms 518.62 ms 61.52 ms
a5ea1aa 453.20 ms 520.36 ms 67.16 ms
3687d4f 441.35 ms 501.16 ms 59.81 ms
9592079 474.51 ms 530.07 ms 55.55 ms

App size

Revision Plain With Sentry Diff
d5bfab8 6.54 MiB 7.53 MiB 1016.51 KiB
f84f616 6.54 MiB 7.53 MiB 1016.51 KiB
5848af1 6.54 MiB 7.53 MiB 1017.67 KiB
d99d55b 6.54 MiB 7.53 MiB 1017.81 KiB
2a151e2 6.54 MiB 7.53 MiB 1017.18 KiB
23ff8a6 6.44 MiB 7.50 MiB 1.05 MiB
62f0687 6.44 MiB 7.50 MiB 1.05 MiB
a5ea1aa 6.54 MiB 7.53 MiB 1016.51 KiB
3687d4f 6.54 MiB 7.53 MiB 1017.67 KiB
9592079 6.44 MiB 7.51 MiB 1.06 MiB

Copy link
Contributor

github-actions bot commented May 27, 2025

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1254.80 ms 1269.76 ms 14.95 ms
Size 7.85 MiB 9.45 MiB 1.59 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c1bb00f 1265.14 ms 1290.85 ms 25.71 ms
f172c4d 1350.66 ms 1408.49 ms 57.83 ms
0be962b 1264.10 ms 1281.16 ms 17.06 ms
6325c3b 1266.52 ms 1291.06 ms 24.54 ms
e1897c5 1252.61 ms 1276.48 ms 23.87 ms
aa950e9 1275.17 ms 1295.33 ms 20.16 ms
a49594a 1284.83 ms 1313.29 ms 28.45 ms
1131914 1277.20 ms 1300.20 ms 23.00 ms
7659cbe 1246.70 ms 1265.88 ms 19.17 ms
3f23617 1261.93 ms 1286.10 ms 24.17 ms

App size

Revision Plain With Sentry Diff
c1bb00f 8.09 MiB 9.07 MiB 1001.06 KiB
f172c4d 8.33 MiB 9.62 MiB 1.29 MiB
0be962b 8.10 MiB 9.16 MiB 1.07 MiB
6325c3b 8.16 MiB 9.17 MiB 1.01 MiB
e1897c5 8.42 MiB 9.91 MiB 1.49 MiB
aa950e9 8.16 MiB 9.17 MiB 1.01 MiB
a49594a 8.16 MiB 9.16 MiB 1.00 MiB
1131914 8.16 MiB 9.17 MiB 1.01 MiB
7659cbe 8.42 MiB 9.89 MiB 1.47 MiB
3f23617 8.16 MiB 9.17 MiB 1.01 MiB

Previous results on branch: feat/imporve-user-feedback-widget

Startup times

Revision Plain With Sentry Diff
3687d4f 1264.86 ms 1281.39 ms 16.53 ms
09c60a5 1247.39 ms 1270.30 ms 22.91 ms
2a151e2 1243.96 ms 1255.74 ms 11.79 ms
62f0687 1244.63 ms 1263.07 ms 18.44 ms
23ff8a6 1241.63 ms 1260.20 ms 18.57 ms
5848af1 1242.07 ms 1258.90 ms 16.83 ms
d99d55b 1266.29 ms 1274.09 ms 7.79 ms
a5ea1aa 1234.21 ms 1243.83 ms 9.63 ms
f84f616 1254.57 ms 1281.06 ms 26.49 ms
d5bfab8 1264.50 ms 1277.04 ms 12.54 ms

App size

Revision Plain With Sentry Diff
3687d4f 7.85 MiB 9.42 MiB 1.57 MiB
09c60a5 8.43 MiB 10.05 MiB 1.62 MiB
2a151e2 7.85 MiB 9.45 MiB 1.59 MiB
62f0687 8.43 MiB 10.04 MiB 1.61 MiB
23ff8a6 8.43 MiB 10.04 MiB 1.61 MiB
5848af1 7.85 MiB 9.42 MiB 1.57 MiB
d99d55b 7.85 MiB 9.45 MiB 1.59 MiB
a5ea1aa 7.85 MiB 9.45 MiB 1.59 MiB
f84f616 7.85 MiB 9.45 MiB 1.59 MiB
d5bfab8 7.85 MiB 9.45 MiB 1.59 MiB

@denrase denrase marked this pull request as ready for review May 28, 2025 15:28
@denrase denrase requested a review from ueman May 28, 2025 15:28
@denrase denrase requested a review from buenaflor June 16, 2025 09:43
Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

I just tested it and did the following

  • Opened feedback
  • Add my name
  • Add my email
  • Add text
  • Tap Capture Screenshot button
  • Feedback screen hides
  • Take Screenshot
  • Feedback screen shows

-> my text input (name, email, body) is gone

So we should make it so the input stays after capturing a screenshot

@denrase
Copy link
Collaborator Author

denrase commented Jun 23, 2025

@buenaflor Ok, then we need to implement some kind of (memory) persistence.

@ueman
Copy link
Collaborator

ueman commented Jun 23, 2025

@denrase Did you look into the RestorationManager and friends? Maybe that already solves the problem without any memory persistence. Alternatively, you can just store the data in a singleton and clear data on feedback submission.

@denrase
Copy link
Collaborator Author

denrase commented Jun 23, 2025

@ueman Will check it out, thx for the tip. Just static state within the widget that we then clear was also the first thing that came to my mind.

@denrase denrase requested a review from buenaflor June 23, 2025 09:22
@denrase
Copy link
Collaborator Author

denrase commented Jun 24, 2025

@ueman Checked the docs for SR. It is described that unique ID on iOS on the project level has to be set in the State Restoration on iOS section. I'm assuming that app developers would need to do that. I'm not sure this is useful to us if thats the way it has to be implemented. Did you use the API?

@denrase denrase requested a review from buenaflor June 25, 2025 06:54
@ueman
Copy link
Collaborator

ueman commented Jun 25, 2025

I haven't used the API yet, but the docs indeed imply that the user needs to tke action which is very unfortunate

Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

We want to track usage of the Widget (dev usage, not usage of the user who opens the widget) so we should add something like options.sdk.integrations('MobileFeedbackWidget');

Maybe we find a way to add it even if the user hasn't opened the widget yet - if at all possible? not sure

@denrase
Copy link
Collaborator Author

denrase commented Jun 27, 2025

@buenaflor Added the integration when the options are accessed. This is not 100% accurate if we want to track if developers are using the feature, but it's the closest we can get with the current structure.

@denrase denrase requested a review from buenaflor June 27, 2025 07:39

/// Options for the [SentryFeedbackWidget]
SentryFeedbackOptions get feedback {
sdk.addIntegration('MobileFeedbackWidget');
Copy link
Contributor

Choose a reason for hiding this comment

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

won't this keep adding the integration if you access the feedback multiple times

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thought so too, but the addIntegration method already checks for this:

// Adds an integration if not already added
void addIntegration(String integration) {
  if (_integrations.contains(integration)) {
    return;
  }
  _integrations.add(integration);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

okay cool

also let's add a small comment why we add this here.

looks confusing without context

@denrase denrase requested a review from buenaflor June 27, 2025 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UserFeedback Widget improvements
3 participants