Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f8dc2b4. Configure here.
The app enjoyment / rating prompt fired straight from app start, outside modal arbitration, so it could land on top of another modal. Add AppRatingPromptEvaluator at priority 6, gated by the new appRatingPromptModal flag. The flag defaults to TRUE: the evaluator owns the decision and the prompt competes with the other modals. Disabling it reverts to AppEnjoymentAppCreationObserver deciding on every app start, unarbitrated. Exactly one of the two paths runs, so the prompt is never evaluated twice in a single start. Triggers on NTP_RENDER rather than APP_RESUME. The dialog is hosted by BrowserActivity, which drops it unless resumed, so an APP_RESUME pass landing on another screen would consume the modal slot without the user seeing anything. An NTP render only fires from a visible browser tab. Move cookie_popup_opt_in to priority 7 to make room at 6. Co-Authored-By: Claude Opus 5 <[email protected]>
f8dc2b4 to
ced1846
Compare
| featureName = "appRatingPromptModal", | ||
| ) | ||
| interface AppRatingPromptModalFeature { | ||
| @Toggle.DefaultValue(DefaultFeatureValue.TRUE) |
There was a problem hiding this comment.
Should we have the default value false for now and have this enabled internally by default? And then when we want to release it we can update the value in the privacy config. Or is this true only for testing?
There was a problem hiding this comment.
I think we can just enable and disable if something goes wrong, there's no reason to wait, and internal testing won't give us much value as it mainly affects new user over a long period of time.
| return@withContext ModalEvaluator.EvaluationResult.Skipped | ||
| } | ||
|
|
||
| val promptType = promptTypeDecider.determineInitialPromptType() |
There was a problem hiding this comment.
I see that in the AppEnjoymentAppCreationObserver we are also checking for the value of the preventDialogQueuingFeature. Do we need that here as well? Or what was the purpose of it?
There was a problem hiding this comment.
we need the prompt type to trigger the prompt.
appEnjoymentPromptEmitter.promptType.value = promptType
The preventDialogQueuingFeature is redundant, and no-op here, but we need to keep since we have a Feature Flag to revert back.
|
|
||
| override val priority: Int = PRIORITY | ||
| override val evaluatorId: String = "app_rating_prompt" | ||
| override val trigger: ModalTrigger = ModalTrigger.NTP_RENDER |
There was a problem hiding this comment.
Before this change the prompt was evaluated on app start. Is there a reason why we have this only for NTP render? The valus that is being set is observer in the browser tab view model.
There was a problem hiding this comment.
Yeah it's up for discussion, I raised it here: https://app.asana.com/1/137249556945/task/1217653553813424/comment/1212839176824586?focus=true
I think this is more appropriate than app start, and I don't think it will affect the frequency of this prompt. It might be that at the time of creating this prompt, that was the best option they had, but that's not the case now.
catalinradoiu
left a comment
There was a problem hiding this comment.
I tested the changes in this PR and the functionality works as expected. Just some clarifying questions but other than this it looks good.
Task/Issue URL: https://app.asana.com/1/137249556945/project/72649045549333/task/1218278045050709?focus=true
Tech Design URL (if applicable):
API Proposals URL(s) (if applicable):
Description
Integrate the rate-our-app prompt into the prompt coordinator, use a feature flag default ON to switch back to old behaviour.
Steps to test this PR
The following command will modify the code so that only one DDG search is needed to meet the thresholds.
Then build, and install.
Perform one search and observe the dialog showing.
Reset app add data, change flag value to False, then repeat.
Note
Medium Risk
Changes when and how the rating prompt appears (NTP vs app start) and modal arbitration order; mitigated by a default-on remote flag that restores legacy behavior when disabled.
Overview
Routes the app enjoyment / rating prompt through the prompts coordinator instead of firing unarbitrated on every app start.
When remote flag
appRatingPromptModalis on (default), newAppRatingPromptEvaluatordecides whether to show the prompt onNTP_RENDERat coordinator priority 6, with a short delay before emitting via the existingAppEnjoymentPromptEmitter.AppEnjoymentAppCreationObserverno-ops on start so the prompt is not evaluated twice or outside arbitration.When the flag is off, behavior reverts to the observer’s app-start path.
CookiePopupOptInEvaluatorpriority moves 6 → 7 so the rating prompt can win ordering against the cookie opt-in modal.Reviewed by Cursor Bugbot for commit ced1846. Bugbot is set up for automated code reviews on this repo. Configure here.