feat(feedback): implement shake gesture detection#7579
feat(feedback): implement shake gesture detection#7579
Conversation
The useShakeGesture configuration property existed but was not implemented. This adds SentryShakeDetector which swizzles UIWindow.motionEnded:withEvent: to detect shake gestures and wires it into SentryUserFeedbackIntegrationDriver. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Internal Changes 🔧Deps
Samples
Other
🤖 This preview updates automatically when you update the PR. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7579 +/- ##
=============================================
- Coverage 85.325% 85.072% -0.254%
=============================================
Files 483 484 +1
Lines 28785 28826 +41
Branches 12506 12507 +1
=============================================
- Hits 24561 24523 -38
- Misses 4177 4256 +79
Partials 47 47
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
…it targets Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…non-iOS The @interface declaration was wrapped in #if TARGET_OS_IOS, causing a compile error on macOS/tvOS/watchOS where the @implementation in the #else block could not find the interface. The class is now declared on all platforms with the methods documented as no-ops on non-iOS. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use monotonic clock (CACurrentMediaTime) instead of NSDate, atomic_bool for thread-safe enabled flag, bridged notification constant, and unconditional cleanup in deinit.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
antonis
requested review from
itaybre,
noahsmartin,
philipphofmann and
philprime
as code owners
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Sources/Swift/Integrations/UserFeedback/SentryUserFeedbackIntegrationDriver.swift
Show resolved
Hide resolved
Track whether this driver instance enabled shake detection and only call disable in deinit if it did. Prevents an old driver's deallocation from disabling shake detection that a new driver already re-enabled during SDK restart.
Sources/Swift/Integrations/UserFeedback/SentryUserFeedbackIntegrationDriver.swift
Show resolved
Hide resolved
The init early-returns when there are no connected scenes (SwiftUI apps). Move observeScreenshots and observeShakeGesture calls before the return so shake detection works in SwiftUI apps.
Sources/Sentry/SentryShakeDetector.m
Outdated
| static void | ||
| sentry_motionEnded(UIWindow *self, SEL _cmd, UIEventSubtype motion, UIEvent *event) | ||
| { | ||
| if (atomic_load_explicit(&_shakeDetectionEnabled, memory_order_acquire) |
There was a problem hiding this comment.
Q: does this run on every shake after calling disable?
There was a problem hiding this comment.
Yes, after disable, sentry_motionEnded still runs on every motionEnded:withEvent:, but it's a single atomic bool check that returns immediately. The swizzle is intentionally never removed (un-swizzling is unsafe with multiple swizzlers).
Add a "Shake to Report" section to the user feedback setup page and update the `useShakeGesture` config description to note it's iOS-only. Ref: getsentry/sentry-cocoa#7579 Co-Authored-By: Claude <noreply@anthropic.com>
| * Swizzling is performed at most once regardless of how many times @c +enable is called. | ||
| * On non-iOS platforms (macOS, tvOS, watchOS), these methods are no-ops. | ||
| */ | ||
| @interface SentryShakeDetector : NSObject |
There was a problem hiding this comment.
m: Any reason in particular to make it in ObjC?
I don't see anything that couldn't be done in Swift here
There was a problem hiding this comment.
Not really other than initially implementing on the RN side and moving it here 😓 I'll convert to draft and move this to Swift. thank you for the feedback 🙇
Sources/Sentry/SentryShakeDetector.m
Outdated
|
|
||
| #else | ||
|
|
||
| NSNotificationName const SentryShakeDetectedNotification = @"SentryShakeDetected"; |
There was a problem hiding this comment.
m: You can place this outside the #if TARGET_OS_IOS preprocessor directive to avoid declaring it twice
Sources/Sentry/SentryShakeDetector.m
Outdated
| #import <objc/runtime.h> | ||
| #import <stdatomic.h> | ||
|
|
||
| #if TARGET_OS_IOS |
There was a problem hiding this comment.
h: iPad also supports shake gestures too
| } | ||
|
|
||
| @objc func handleShakeGesture() { | ||
| guard !displayingForm else { return } |
There was a problem hiding this comment.
h: on iPad there may be several windows, and I am not sure if all of them get the shake gesture or not.
Will this present the feedback form on all of them?
Replaces the ObjC implementation with a Swift class while maintaining full ObjC compatibility via @objc annotations. The class name remains SentryShakeDetector in ObjC, preserving compatibility with sentry-react-native.
Make SentryShakeDetector an open class to match the ObjC API and regenerate sdk_api.json with Xcode 16.4.
The class is a static utility not meant to be subclassed. Regenerate API snapshot accordingly.
| deinit { | ||
| customButton?.removeTarget(self, action: #selector(showForm(sender:)), for: .touchUpInside) | ||
| if didEnableShakeDetection { | ||
| SentryShakeDetector.disable() |
There was a problem hiding this comment.
l: I think if it safe to always call disable
Always call SentryShakeDetector.disable() in deinit instead of tracking whether this instance enabled it.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Shake in early-return path permanently blocks future shakes
- showForm(screenshot:) now early-returns when no presenter exists, preventing displayingForm from being set true when presentation cannot occur.
Or push these changes by commenting:
@cursor push 3846cd567d
Preview (3846cd567d)
diff --git a/Sources/Swift/Integrations/UserFeedback/SentryUserFeedbackIntegrationDriver.swift b/Sources/Swift/Integrations/UserFeedback/SentryUserFeedbackIntegrationDriver.swift
--- a/Sources/Swift/Integrations/UserFeedback/SentryUserFeedbackIntegrationDriver.swift
+++ b/Sources/Swift/Integrations/UserFeedback/SentryUserFeedbackIntegrationDriver.swift
@@ -120,11 +120,14 @@
@available(iOSApplicationExtension, unavailable)
private extension SentryUserFeedbackIntegrationDriver {
func showForm(screenshot: UIImage?) {
+ guard let presenter else {
+ return
+ }
let form = SentryUserFeedbackFormController(config: configuration, delegate: self, screenshot: screenshot)
form.presentationController?.delegate = self
widget?.rootVC.setWidget(visible: false, animated: configuration.animations)
displayingForm = true
- presenter?.present(form, animated: configuration.animations) {
+ presenter.present(form, animated: configuration.animations) {
self.configuration.onFormOpen?()
}
}| */ | ||
| if UIApplication.shared.connectedScenes.isEmpty && UIApplication.shared.delegate == nil { | ||
| observeScreenshots() | ||
| observeShakeGesture() |
There was a problem hiding this comment.
Shake in early-return path permanently blocks future shakes
Medium Severity
In the early-return path (SwiftUI apps with no connected scenes and no delegate), observeShakeGesture() registers for shake notifications but widget is nil and there's no customButton, so presenter is nil. When a shake triggers handleShakeGesture, it calls showForm(screenshot:) which unconditionally sets displayingForm = true before calling presenter?.present(...). Since presenter is nil, the form is never presented, so displayingForm is never reset to false. All subsequent shakes are permanently blocked by the guard !displayingForm check. Additionally, if showWidget() is later called, the widget window's hitTest sees displayingForm == true and intercepts all touches instead of passing through non-button taps.
Additional Locations (2)
There was a problem hiding this comment.
I think this is a pre-existing edge case. The same behavior applies to the screenshot trigger on this path.
There was a problem hiding this comment.
@antonis Do you think we should fix it for both cases? at least for this PR we do it correctly?
| /// Enables shake gesture detection. On iOS/iPadOS, swizzles `UIWindow.motionEnded(_:with:)` | ||
| /// the first time it is called, and from then on posts `.SentryShakeDetected` | ||
| /// whenever a shake is detected. No-op on non-iOS platforms. | ||
| public static func enable() { |
There was a problem hiding this comment.
m: Please consider adding more SDK debug logging to all the code branches in this method, as swizzling can be tricky and have side-effects
| */ | ||
| if UIApplication.shared.connectedScenes.isEmpty && UIApplication.shared.delegate == nil { | ||
| observeScreenshots() | ||
| observeShakeGesture() |
There was a problem hiding this comment.
@antonis Do you think we should fix it for both cases? at least for this PR we do it correctly?
| if configuration.useShakeGesture { | ||
| SentryShakeDetector.enable() | ||
| NotificationCenter.default.addObserver(self, selector: #selector(handleShakeGesture), name: .SentryShakeDetected, object: nil) | ||
| } |
There was a problem hiding this comment.
l: Debug logs could be valuable
| if configuration.useShakeGesture { | |
| SentryShakeDetector.enable() | |
| NotificationCenter.default.addObserver(self, selector: #selector(handleShakeGesture), name: .SentryShakeDetected, object: nil) | |
| } | |
| guard configuration.useShakeGesture else { | |
| SentrySDKLog.debug("Received shake gesture while disabled) | |
| } | |
| SentryShakeDetector.enable() | |
| NotificationCenter.default.addObserver(self, selector: #selector(handleShakeGesture), name: .SentryShakeDetected, object: nil) |
| /// Disables shake gesture detection. Does not un-swizzle `UIWindow`; it only suppresses | ||
| /// the notification so the overhead is negligible. No-op on non-iOS platforms. | ||
| public static func disable() { | ||
| enabled = false |
There was a problem hiding this comment.
m: Can we implement a unswizzling logic here?
4 participants
📜 Description
Implements shake gesture detection for the user feedback form. When
useShakeGestureis enabled inSentryUserFeedbackConfiguration, shaking the device opens the feedback form.The implementation swizzles
UIWindow.motionEnded:withEvent:usingclass_addMethod+method_setImplementationto safely intercept shake events without modifyingUIResponder's inherited implementation. A cooldown of 1 second prevents duplicate triggers.Note: this is exposed to be used on React Native too getsentry/sentry-react-native#5754
💡 Motivation and Context
The
useShakeGestureproperty already existed inSentryUserFeedbackConfigurationbut was never wired up. This PR implements the feature. The implementation is placed here (sentry-cocoa) rather than in each SDK separately so it can be reused by all SDKs that embed the feedback UI (React Native, Flutter, .NET MAUI, Unity).💚 How did you test it?
SentryShakeDetector📝 Checklist
sendDefaultPIIis enabled.Closes #7597