Performance: integration branch for live validation - #1354
Conversation
|
@codex review |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR reduces PHP memory use and database work across admin loading, popup and theme caching, subscriber and form queries, analytics counters, remote responses, and CSS delivery. It also adds extensive query-count, cache, authorization, and fallback tests. ChangesPerformance optimization
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
classes/Utils/I10n.php (1)
45-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the non-
WP_Errorfailure paths.The new branches at Line 45 through Line 65 cache failures for non-200 responses, empty bodies, invalid JSON, and missing
translations.tests/php/tests/Remote_Cache_Test.phpLines 48 through 67 only exercisesWP_Error. Add cases for each response shape and assert that the second call avoids HTTP and returns the cached empty array.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@classes/Utils/I10n.php` around lines 45 - 65, Extend Remote_Cache_Test to cover the non-WP_Error failure branches in the translation-fetching method: non-200 responses, empty response bodies, invalid JSON, and missing or non-array translations. For each response shape, call the method twice, assert the result is the cached empty array, and verify the second call does not perform another HTTP request.tests/php/tests/Remote_Cache_Test.php (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the strict-types declaration.
This new PHP test file does not declare
strict_types=1. Add the declaration after<?php.As per coding guidelines: “Use strict typing when possible: declare(strict_types=1);”.
Proposed change
<?php +declare(strict_types=1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/php/tests/Remote_Cache_Test.php` at line 1, Add the PHP strict-types declaration immediately after the opening tag in Remote_Cache_Test.php, using declare(strict_types=1);.Source: Coding guidelines
classes/Controllers/WP/Dashboard.php (1)
144-184: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the popup scan and the metadata priming.
The query uses
posts_per_page => -1, so the widget loads every published, enabled popup that has views.update_meta_cache( 'post', $popup_ids )then primes every meta row for all of those posts. On sites with many popups this keeps a large meta cache in memory for one dashboard widget.Consider a bounded result set, or chunked meta priming.
♻️ Example: prime metadata in chunks
- update_meta_cache( 'post', $popup_ids ); + foreach ( array_chunk( $popup_ids, 100 ) as $popup_id_chunk ) { + update_meta_cache( 'post', $popup_id_chunk ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@classes/Controllers/WP/Dashboard.php` around lines 144 - 184, Bound the popup query and metadata priming in the dashboard statistics flow around $query and $popup_ids. Replace the unbounded posts_per_page => -1 scan with a bounded result set, or retain the full result set only if update_meta_cache is changed to process $popup_ids in fixed-size chunks. Preserve the existing empty-result response and subsequent statistics behavior.classes/Model/Theme.php (1)
25-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the global hook registration out of the model constructor.
The constructor registers three global meta hooks and guards them with
has_action( 'added_post_meta', $callback ). The guard checks one hook only. If any code removes just that callback, the next constructed theme re-registersupdated_post_metaanddeleted_post_metaa second time, and the generation counter then advances twice per write. Registration also depends on a theme object being constructed, so native meta writes before the first construction do not bump the counter.Register these hooks once during plugin initialization instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@classes/Model/Theme.php` around lines 25 - 35, Remove the meta-hook registration and has_action guard from Theme::__construct. Register the added_post_meta, updated_post_meta, and deleted_post_meta callbacks once during plugin initialization, using the existing invalidate_settings_caches_on_meta_change callback, so hooks are active before any Theme instance is constructed and are not duplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@classes/Controllers/Assets.php`:
- Around line 652-658: The popup choices filter in Assets.php must reject
published non-popup posts; update the validation around the popup iteration to
require get_post_type( $popup ) === 'popup' alongside the existing WP_Post and
published-status checks. In tests/php/tests/Assets_Controller_Test.php lines
131-180 and 280-329, inject a published non-popup post and assert it is excluded
from the block-editor and Components choices respectively.
In `@classes/Controllers/Frontend/Popups.php`:
- Around line 98-101: Update the clean_post_cache handling in the controller
registration and invalidate the modern popup repository cache in addition to the
frontend cache. Extend or call invalidate_queried_popup() so the relevant
Repository::$items_by_id entry is removed when a popup post cache is cleaned,
while preserving existing frontend invalidation behavior.
In `@classes/Controllers/WP/Dashboard.php`:
- Around line 193-214: Update the top-performer calculation in the popup-ranking
loop to use the selected `$top_rate` for `$top_performer_rate` instead of
recomputing it from `$top_conversions` and `$top_views`, so the displayed
percentage matches the rate used by the ranking conditions.
In `@classes/Helpers.php`:
- Around line 377-383: Update the post-status handling around the visible status
validation and the subsequent query construction so post_status => 'any' is
either rejected consistently with unsupported statuses or honored by passing
through the requested statuses and filtering results accordingly; do not allow
it through validation while later forcing publish in the affected query path.
In `@includes/functions/popups/queries.php`:
- Around line 44-46: Update the caching condition around pum_is_popup() to also
require that $popup_controller is non-null before calling cache_queried_popup().
Reuse the existing controller null-check pattern from the surrounding query
logic, preserving the current admin and popup guards.
In `@tests/php/tests/PUM_Admin_Shortcode_UI_Test.php`:
- Around line 52-55: Make the stylesheet count assertion in the
block_editor_settings test RTL-aware: expect four entries when is_rtl() is true
and two otherwise. Keep the existing CSS content assertions unchanged.
In `@tests/php/tests/PUM_DB_Subscribers_Test.php`:
- Around line 212-230: Update test_create_table_adds_created_index to avoid the
unsupported %i placeholder on WordPress versions before 6.2, reusing the pre-6.2
identifier-query fallback from Subscribers DB handling; alternatively, add a
WordPress-version guard that skips this test on older versions.
In `@tests/php/tests/Remote_Cache_Test.php`:
- Around line 16-21: Initialize the shared transients before each test by adding
a setUp() method that calls parent::setUp() and deletes both pum_plugin_notices
and pum_alerts_translation_status before tests run. Keep the existing tearDown()
cleanup unchanged.
---
Nitpick comments:
In `@classes/Controllers/WP/Dashboard.php`:
- Around line 144-184: Bound the popup query and metadata priming in the
dashboard statistics flow around $query and $popup_ids. Replace the unbounded
posts_per_page => -1 scan with a bounded result set, or retain the full result
set only if update_meta_cache is changed to process $popup_ids in fixed-size
chunks. Preserve the existing empty-result response and subsequent statistics
behavior.
In `@classes/Model/Theme.php`:
- Around line 25-35: Remove the meta-hook registration and has_action guard from
Theme::__construct. Register the added_post_meta, updated_post_meta, and
deleted_post_meta callbacks once during plugin initialization, using the
existing invalidate_settings_caches_on_meta_change callback, so hooks are active
before any Theme instance is constructed and are not duplicated.
In `@classes/Utils/I10n.php`:
- Around line 45-65: Extend Remote_Cache_Test to cover the non-WP_Error failure
branches in the translation-fetching method: non-200 responses, empty response
bodies, invalid JSON, and missing or non-array translations. For each response
shape, call the method twice, assert the result is the cached empty array, and
verify the second call does not perform another HTTP request.
In `@tests/php/tests/Remote_Cache_Test.php`:
- Line 1: Add the PHP strict-types declaration immediately after the opening tag
in Remote_Cache_Test.php, using declare(strict_types=1);.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 09475300-029c-4a4d-9723-8b4af074ebff
📒 Files selected for processing (59)
CHANGELOG.mdassets/js/src/admin/settings-page/index.jsclasses/Admin.phpclasses/Admin/Ajax.phpclasses/Admin/Assets.phpclasses/Admin/Notices.phpclasses/Admin/Pages.phpclasses/Admin/Popups.phpclasses/Admin/Settings.phpclasses/Admin/Shortcode/UI.phpclasses/Admin/Subscribers/Table.phpclasses/Base/Service/Repository.phpclasses/Controllers/Assets.phpclasses/Controllers/Frontend/Popups.phpclasses/Controllers/WP/Dashboard.phpclasses/DB/Subscribers.phpclasses/Helpers.phpclasses/Integration/Form/ContactForm7.phpclasses/Integration/Form/Elementor.phpclasses/Integration/Form/HappyForms.phpclasses/Integration/Form/KaliForms.phpclasses/Integration/Form/WPForms.phpclasses/Model/Theme.phpclasses/Privacy.phpclasses/Repository/Popups.phpclasses/Services/FormConversionTracking.phpclasses/Services/LinkClickTracking.phpclasses/Services/Notifications/FeatureAnnouncements.phpclasses/Services/Repository/CallToActions.phpclasses/Services/Repository/Popups.phpclasses/Utils/AnalyticsCounter.phpclasses/Utils/I10n.phpdocs/php-performance-optimization-retrospective.mdincludes/functions/popups/queries.phpincludes/integrations/class-pum-cf7.phpincludes/integrations/class-pum-gravity-forms.phptests/php/fixtures/class-elementor-submissions-query.phptests/php/fixtures/class-pum-test-feature-announcements.phptests/php/tests/Assets_Controller_Test.phptests/php/tests/CallToActions_Repository_Test.phptests/php/tests/Dashboard_Controller_Test.phptests/php/tests/Elementor_Form_Query_Test.phptests/php/tests/Feature_Announcements_Query_Test.phptests/php/tests/FormConversionTracking_Test.phptests/php/tests/Form_Integration_Query_Test.phptests/php/tests/Frontend_Popups_Controller_Test.phptests/php/tests/LinkClickTracking_Test.phptests/php/tests/PUM_Admin_Assets_Test.phptests/php/tests/PUM_Admin_Loader_Test.phptests/php/tests/PUM_Admin_Popups_Test.phptests/php/tests/PUM_Admin_Settings_Ajax_Test.phptests/php/tests/PUM_Admin_Settings_Test.phptests/php/tests/PUM_Admin_Shortcode_UI_Test.phptests/php/tests/PUM_Admin_Subscribers_Table_Test.phptests/php/tests/PUM_DB_Subscribers_Test.phptests/php/tests/PUM_Helpers_Test.phptests/php/tests/PUM_Model_Theme_Test.phptests/php/tests/Popups_Repository_Title_Choices_Test.phptests/php/tests/Remote_Cache_Test.php
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74dccf2f74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@coderabbitai review |
|
|
@codex review |
|
@coderabbitai review |
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
5e0587c to
9ad07e9
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
4432e7d to
7523f92
Compare
|
@codex review |
|
@coderabbitai full review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7523f92614
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7523f92 to
f9ccac1
Compare
|
@codex review |
|
@coderabbitai full review |
|
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
f9ccac1 to
0084912
Compare
|
@codex review |
|
@coderabbitai full review |
|
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
efe151b to
1356af7
Compare
|
@codex review |
1 similar comment
|
@codex review |
1356af7 to
13e2db2
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13e2db23b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
1 similar comment
|
@codex review |
4d0b756 to
4d55c36
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
e669881 to
b62bb80
Compare
|
@codex review |
a7848d2 to
9e0b4ce
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Closing the live-validation draft: its only remaining implementation commit was merged through #1317. This branch was never intended to merge directly. |
1 participant
Purpose
Draft-only live-local test branch for the two remaining popup-query/editor performance changes. Never merge this aggregate PR directly.
Exact composition
develop@c972387c2b5fb5e76ba8467cf9d6c34127b9d9b6(merged Harden lightweight popup selector queries #1356 shared popup-selector hardening)e2a13182caf988073225f63c20ca683cfd485133826092dc637b30c02feb5d05cca04cb43f1e2a6fe2a13182caf988073225f63c20ca683cfd485133,0084912a5b5934e43d10a9f418fd2f88c6bb99570084912a5b5934e43d10a9f418fd2f88c6bb9957626f8a2c7633787a9e803d92bd3e96c6047accdf4e1939cbabba219163c3e75efc9b19261d6cc300) because its parent is the included Performance: reduce memory and TTFB on Popup Maker admin pages #1316 commit.Explicit exclusions
developare inherited only through the exact base.Local verification on this exact aggregate tree
git diff --check: passed.get_posts(), directWP_Query,$wpdb, or per-popup validation.Live-local smoke focus
This PR must remain a draft and must not be merged.