-
Notifications
You must be signed in to change notification settings - Fork 1
Add filter to opt out of opt-in #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @westonruter, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses a user request by providing a new filter that allows site administrators to bypass the 'Remember Me' checkbox as a requirement for enabling browser back/forward cache (bfcache) for authenticated users. This change offers greater flexibility, enabling bfcache for all logged-in users by default if desired, and also ensures that the associated UI elements are only displayed when the opt-in mechanism is active.
Highlights
- New Filter for bfcache Opt-in Control: I've introduced a new filter,
nocache_bfcache_use_remember_me_as_opt_in, which allows developers to control whether the 'Remember Me' checkbox on the login screen is used as an explicit opt-in mechanism for browser back/forward cache (bfcache). - Bypass 'Remember Me' Requirement: When the
nocache_bfcache_use_remember_me_as_opt_infilter is set tofalse, bfcache will be enabled for all logged-in users by default (assuming JavaScript is enabled), bypassing the need for them to check the 'Remember Me' checkbox. - Conditional UI Display: The UI elements associated with the bfcache opt-in, such as the sparkle emoji and popover on the login screen, will now be conditionally displayed. If the 'Remember Me' opt-in is bypassed via the new filter, these UI elements will not be shown.
- Documentation Update: The
README.mdfile has been updated to clearly document the purpose and usage of the newnocache_bfcache_use_remember_me_as_opt_infilter, including a practical PHP code example.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new filter, nocache_bfcache_use_remember_me_as_opt_in, which allows site administrators to enable bfcache for all logged-in users, bypassing the need to check the "Remember Me" box. The implementation is straightforward and correctly applies the new filter to control bfcache behavior. The README has also been updated to reflect this new functionality.
My review found the code to be well-written, but I have one minor suggestion to improve long-term maintainability by updating a documentation placeholder.
| * | ||
| * @param bool $enabled Whether the "Remember Me" checkbox on the login screen is used as an opt-in to bfcache. | ||
| */ | ||
| return (bool) apply_filters( 'nocache_bfcache_use_remember_me_as_opt_in', true ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gemini-code-assist Is this filter name ideal? Is it better for a filter to have a prefix followed by a verb like "use" or to follow with the noun of the thing being filtered? For example, this could instead be nocache_bfcache_remember_me_opt_in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question, @westonruter! Both options are quite clear, but I think nocache_bfcache_use_remember_me_as_opt_in is slightly more ideal.
The reason is that the filter is determining whether the "Remember Me" checkbox is used as an opt-in. The inclusion of the verb "use" directly reflects the action or behavior being controlled by the filter, aligning well with the is_remember_me_used_as_opt_in() function it's associated with. It clearly communicates that the filter is about enabling or disabling this specific functionality.
While nocache_bfcache_remember_me_opt_in is more concise, it leans towards describing the state or noun of the opt-in, rather than the action of using it. In the context of a boolean filter that enables or disables a feature, the more explicit verb often provides better clarity for developers looking to modify the behavior.
So, I'd lean towards keeping the current name for its explicit clarity regarding the action being filtered.
Per a topic on the support forum:
Ultimately, the default value for this filter may switch from
truetofalse. It remains opt-in for now to give the user control of being able to turn off bfcache by not checking the "Remember Me" checkbox, in case there is some issue it causes. The opt-in also provides an opportunity to announce/promote the feature.