refactor: update hook check in asset enqueuing#58
Merged
Conversation
Change the string check in the `enqueue_assets` function from 'stracini-visitor-analytics' to 'sva-'. This update helps broaden the scope and future-proofs the function for additional sub-pages or features that may have 'sva-' namespaces. Additionally, this ensures that asset enqueuing applies consistently across new admin pages related to the plugin.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the admin asset enqueue guard so that assets are loaded for any admin page hook containing the broader 'sva-' namespace instead of the previous, more specific 'stracini-visitor-analytics' string, ensuring assets are applied across current and future plugin admin pages. Flow diagram for updated enqueue_assets hook checkflowchart TD
A["enqueue_assets(hook)"] --> B{"str_contains(hook, 'sva-')"}
B -- "No" --> C["return"]
B -- "Yes" --> D["Enqueue admin assets"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughIn ChangesAdmin Asset Enqueue Filter
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Broadening the hook check to
str_contains( $hook, 'sva-' )may match unintended admin pages that happen to includesva-in their hook; consider tightening this to a more specific pattern (e.g., a known prefix or exact slug pattern) to avoid accidental matches. - Since
'sva-'is now the logical namespace for admin pages, consider extracting it into a class constant or shared configuration to avoid future magic strings and keep the namespace consistent across the codebase.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Broadening the hook check to `str_contains( $hook, 'sva-' )` may match unintended admin pages that happen to include `sva-` in their hook; consider tightening this to a more specific pattern (e.g., a known prefix or exact slug pattern) to avoid accidental matches.
- Since `'sva-'` is now the logical namespace for admin pages, consider extracting it into a class constant or shared configuration to avoid future magic strings and keep the namespace consistent across the codebase.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Contributor
🔍 WordPress Plugin Check Report
📊 Report
|
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
399 |
PluginCheck.Security.DirectDB.UnescapedDBParameter | Unescaped parameter $sql used in $wpdb->get_results()\n$sql assigned unsafely at line 394. |
🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check
gstraccini
Bot
added
the
☑️ auto-merge
![gstraccini[bot]](https://avatars.githubusercontent.com/in/480132?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📑 Description
Change the string check in the
enqueue_assetsfunction from 'stracini-visitor-analytics' to 'sva-'. This update helps broaden the scope and future-proofs the function for additional sub-pages or features that may have 'sva-' namespaces. Additionally, this ensures that asset enqueuing applies consistently across new admin pages related to the plugin.✅ Checks
☢️ Does this introduce a breaking change?
Summary by Sourcery
Broaden the admin asset enqueuing hook check to match all plugin-related pages using the generic 'sva-' namespace instead of a specific slug.
Summary by CodeRabbit