fix(feedback): Keep the User Feedback screenshot trigger active after form dismissal#8048
fix(feedback): Keep the User Feedback screenshot trigger active after form dismissal#8048tsushanth wants to merge 8 commits into
Conversation
tsushanth
requested review from
NinjaLikesCheez,
itaybre,
noahsmartin,
philipphofmann and
philprime
as code owners
denrase
left a comment
denrase
left a comment
There was a problem hiding this comment.
Thank you for contributing and finding this! 🥳Left some feedback.
|
Thanks @denrase, good catches on both. Pushed bfa4978:
The merge conflict on the branch is on |
|
Pushed 61dc439 — dropped the For the test side, the file only has one new test in the PR — |
When showFormForScreenshots = true, userCapturedScreenshot() removes the UIApplication.userDidTakeScreenshotNotification observer before presenting the form, so holding the form open doesn't keep re-presenting it. The observer was never re-registered when the form was dismissed, so every subsequent screenshot was silently ignored — the feedback flow only worked once per app launch. Re-register in userFeedbackFormDidClose(_:) and make observeScreenshots() idempotent (it now removes any existing observer first) so the re-register call can't end up attaching two observers and presenting the form twice for a single screenshot. Adds two regression tests against the previously test-uncovered SentryUserFeedbackIntegrationDriver: - testScreenshotTrigger_afterFormDismissal_stillPresentsForm — fails on main with presentCallCount stuck at 1; passes after the fix. - testScreenshotTrigger_afterFormDismissal_presentsFormOnlyOncePerScreenshot — guards against a double-fire regression if observeScreenshots() were ever called twice in the future. Closes getsentry#7641
Per maintainer review, simplify the fix: instead of removing the screenshot observer before presenting and re-registering it after dismissal, guard userCapturedScreenshot on the existing displayingForm flag and leave the observer attached for the integration's lifetime. This mirrors how handleShakeGesture already handles the same race, and covers the cases where there is no presenter for the form or one is already presented without having to think about observer lifecycle. Collapse the two regression tests into one four-step sequence: first screenshot presents once, second screenshot while form is active still shows one presentation, dismiss the form, third screenshot presents a second time. This is the exact scenario reported in getsentry#7641 plus the double-present guard.
denrase noted observing the screenshot notification once at integration start, plus the existing displayingForm guard in userCapturedScreenshot, is enough — no de-register / re-register dance needed. The original implementation matches main again; the only behavior change vs main is the guard.
tsushanth
force-pushed
the
fix/feedback-screenshot-trigger-only-once
branch
from
61dc439 to
647f51a
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
@tsushanth Thank you, I will take a look and push it over the finish line if we need anything else. 🙇 |
denrase
left a comment
denrase
left a comment
There was a problem hiding this comment.
@tsushanth Thank you for contibuting! :bow I have added an additional test and cleaned up a bit. Doing general commit now to give other maintainers a change to review (/cc @philprime ) to have a second pair of eyes.
denrase
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8048 +/- ##
=============================================
+ Coverage 87.401% 87.479% +0.078%
=============================================
Files 559 559
Lines 32170 32172 +2
Branches 13156 13159 +3
=============================================
+ Hits 28117 28144 +27
+ Misses 4004 3981 -23
+ Partials 49 47 -2
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Harness.
|
…shot-trigger-only-once # Conflicts: # Sources/Swift/Integrations/UserFeedback/SentryUserFeedbackIntegrationDriver.swift
3 participants
📜 Description
Fixes
showFormForScreenshots = trueonly working once per app launch.The screenshot observer now stays registered, and screenshot events are ignored while a feedback form is already displayed. The widget early-return path also registers the screenshot observer.
💡 Motivation and Context
Fixes #7641.
Previously,
userCapturedScreenshot()removed the screenshot observer before presenting the form, but it was never re-registered after dismissal.💚 How did you test it?
Added coverage for screenshot behavior during/after form presentation and observer cleanup on driver deinit.
make test-ios IOS_SIMULATOR_OS=18.5 IOS_DEVICE_NAME='iPhone 16e' ONLY_TESTING='SentryTests/UserFeedbackIntegrationTests'📝 Checklist
sendDefaultPIIis enabled.