feat(feedback): Support runtime enable/disable of shake-to-report#5827
feat(feedback): Support runtime enable/disable of shake-to-report#5827markushi wants to merge 5 commits into
Conversation
|
📲 Install BuildsAndroid
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 22f4345 | 325.23 ms | 454.66 ms | 129.43 ms |
| 22f4345 | 312.78 ms | 347.40 ms | 34.62 ms |
| 8558cac | 306.16 ms | 355.24 ms | 49.09 ms |
| bb0ff41 | 317.76 ms | 384.66 ms | 66.90 ms |
| 7c1a728 | 289.46 ms | 368.15 ms | 78.69 ms |
| d501a7e | 314.55 ms | 343.34 ms | 28.79 ms |
| 4fc476b | 280.63 ms | 363.04 ms | 82.42 ms |
| 6727e14 | 337.22 ms | 373.94 ms | 36.71 ms |
| ae7fed0 | 293.84 ms | 380.22 ms | 86.38 ms |
| ee747ae | 400.46 ms | 423.61 ms | 23.15 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 22f4345 | 1.58 MiB | 2.29 MiB | 719.83 KiB |
| 22f4345 | 1.58 MiB | 2.29 MiB | 719.83 KiB |
| 8558cac | 0 B | 0 B | 0 B |
| bb0ff41 | 0 B | 0 B | 0 B |
| 7c1a728 | 0 B | 0 B | 0 B |
| d501a7e | 0 B | 0 B | 0 B |
| 4fc476b | 0 B | 0 B | 0 B |
| 6727e14 | 1.58 MiB | 2.28 MiB | 718.64 KiB |
| ae7fed0 | 1.58 MiB | 2.12 MiB | 551.77 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
…r.setDialog to prevent overlapping feedback dialogs
…e-to-report' into feat/runtime-enable-disable-shake-to-report
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ac4292c. Configure here.
| startDetecting(options); | ||
| } else { | ||
| dialogRequestedShakeDetection = false; | ||
| } |
There was a problem hiding this comment.
Opt-in flag cleared on re-track
Medium Severity
setDialog with startShakeDetection=false always sets dialogRequestedShakeDetection to false, so an opted-in dialog that later calls setDialog(this, false) from onStart loses its keep-alive flag. A subsequent disable() then stops detection even though that dialog is still tracked, which contradicts the controller contract and the intended re-show behavior.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ac4292c. Configure here.
| } | ||
| return; | ||
| } | ||
| dialogRef = new WeakReference<>(dialog); |
There was a problem hiding this comment.
Opted-in dialog only weakly held
High Severity
Per-form opt-in stores the dialog in a WeakReference, unlike the previous lifecycle-callback approach that kept the form alive. A create() call that does not retain the instance can be collected immediately, so a later shake finds no dialog and does nothing, while detection may keep running because nothing clears it when the weak ref goes null.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ac4292c. Configure here.
| if (!resolvedFeedbackOptions.isUseShakeGesture() | ||
| || globalFeedbackOptions.isUseShakeGesture() | ||
| || globalFeedbackOptions.getShakeController().isEnabled()) { | ||
| return; |
There was a problem hiding this comment.
Stale option blocks per-form opt-in
Medium Severity
maybeEnableShakeToShow still bails out when globalFeedbackOptions.isUseShakeGesture() is true, even though that flag is only the initial state and is not cleared by disableFeedbackOnShake(). After a runtime disable following an initially enabled option, a per-form isUseShakeGesture opt-in never calls setDialog, so shake-to-show for that form never starts.
Reviewed by Cursor Bugbot for commit ac4292c. Configure here.
| dialogRequestedShakeDetection = true; | ||
| startDetecting(options); | ||
| } else { | ||
| dialogRequestedShakeDetection = false; |
There was a problem hiding this comment.
Bug: Calling setDialog(..., false) unconditionally resets dialogRequestedShakeDetection to false, causing shake detection to be stopped prematurely when disable() is called.
Severity: MEDIUM
Suggested Fix
Modify the setDialog method. When startShakeDetection is false, do not change the value of dialogRequestedShakeDetection. The flag should only be reset to false when the dialog is cleared (i.e., dialog == null).
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
sentry-android-core/src/main/java/io/sentry/android/core/FeedbackShakeIntegration.java#L120
Potential issue: The `setDialog` method unconditionally sets
`dialogRequestedShakeDetection` to `false` when its `startShakeDetection` parameter is
`false`. This creates a bug in a specific scenario: 1. A form enables per-dialog shake
detection by calling `setDialog(this, true)`. 2. The form's `onStart()` lifecycle method
then calls `setDialog(this, false)` to track its visibility. This incorrectly resets the
`dialogRequestedShakeDetection` flag. 3. If the global shake integration is then
disabled via `disable()`, it will check the flag, find it `false`, and prematurely call
`stopDetecting()`, disabling shake detection for the form that intended to keep it
active.
Also affects:
sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java:242~242


📜 Description
Adds a runtime API to toggle shake-to-report after
Sentry.init():Sentry.feedback().enableFeedbackOnShake()Sentry.feedback().disableFeedbackOnShake()SentryFeedbackOptions.isUseShakeGesture()now only determines the initial state; the runtime API overrides it afterwards. Propagation works similar toIFormHandler/ReplayControllerby introducing a newSentryFeedbackOptions.IShakeController.This also required a bit of re-wiring on-demand Dialogs vs. the automatic ones, so no duplicate Dialogs are shown and no duplicate detectors run at the same time. The dialogs now report back to the
IShakeController, which now is the only component which takes care of starting/stopping shake detection.💡 Motivation and Context
Having a runtime option to enable / disable the feedback.
Closes #5486 (JAVA-539)
💚 How did you test it?
Unit tests + Manual Tests.
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
FeedbackShakeIntegrationso there's a single detector/arbitration point