Add feature flag for the Suggest redirect feature - #9522
Conversation
malmstein
left a comment
malmstein
left a comment
There was a problem hiding this comment.
Move the flag inside AndroidBrowserConfigFeature, it’s where are all the browser related feature flags live and where you’ll see other error page related flags too.
| scope = AppScope::class, | ||
| featureName = "suggestRedirectOnUnresolvedError", | ||
| ) | ||
| interface SuggestRedirectOnUnresolvedErrorFeature { |
There was a problem hiding this comment.
this should live inside AndroidBrowserConfigFeature and not as a separate flag.
There was a problem hiding this comment.
That was my initial choice. However, a new lint, NoNewBrowserFeatureToggle, was introduced last week that prevents new toggles from being added to the AndroidBrowserConfigFeature interface. You may see it (here).
There was a problem hiding this comment.
@malmstein @juandiana Correct, as written in https://app.asana.com/1/137249556945/project/1202561462274611/task/1217462888888561?focus=true we recently moved AndroidBrowserConfigFeature from :app module to a new module to unblock extraction of several features from the :app module.
We want to avoid adding new feature flags to the new module and the guidance is that they should be owned by whatever feature actually needs it. If we strongly believe that a new feature flag belongs in AndroidBrowserConfigFeature then an exception can be made. Do we think this is such a case?
There was a problem hiding this comment.
I missed that task Domen, my bad. That makes total sense then, let's follow the new rules instead and keep this outside.
There was a problem hiding this comment.
I believe the PR is already in good shape. It follows the first case from the article @landomen cited:
One -impl module reads it → put the @ContributesRemoteFeature interface in that -impl module.
Please let me know if I missed something otherwise.
The step gated on `base.ref == default_branch`, but a stacked PR keeps its parent's branch as base.ref even after the merge queue lands its commit directly on develop, so every PR above the root of a stack was skipped. Compare the PR's merge commit against the default branch instead: `behind` or `identical` means the commit is on develop, whatever the PR targeted. Verified against the Suggest-redirect stack (#9522/#9547/#9580, all now resolve true) and against PRs whose commits genuinely never reached develop (they report `diverged`). The compare status is teed to the log so a false result can be told apart from the commit not having reached the branch yet.
3 participants
Task/Issue URL: https://app.asana.com/1/137249556945/project/1216807998862658/task/1217572324167492?focus=true
Description
Add the feature flag interface inside the package where the feature will reside. Default value set to
false.Steps to test this PR
suggestRedirectOnUnresolvedErrortoggle and itssuggestRedirectsub-feature toggle are present.UI changes
N/A
Note
Low Risk
Toggle-only addition with no navigation or error-page logic changes; risk is limited to feature inventory registration.
Overview
Introduces
SuggestRedirectOnUnresolvedErrorFeatureinapp/browser/suggestredirectas a remote-controllable flag for an upcoming behavior: on unresolved bare-domain errors, optionally offer a link to thewwwsubdomain when it resolves.The feature is registered as
suggestRedirectOnUnresolvedErrorwithself()and asuggestRedirect()sub-toggle; both default to off. No user-facing behavior is wired in this PR—only inventory/toggle plumbing for later rollout.Reviewed by Cursor Bugbot for commit 0879433. Bugbot is set up for automated code reviews on this repo. Configure here.