Conversation
The [pum_cta] shortcode was not receiving block button styles because it used different CSS classes than the block editor. This caused sharp corners in Classic editor popups and full-width buttons in Block editor popups. - Add block-compatible classes to shortcode output (wp-block-popup-maker-cta-button, wp-block-popup-maker-cta-button__link) - Create dedicated shortcode styles that load for all popups - Fix block CSS width rule to only apply within block container Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughThis PR introduces comprehensive form submission and link click conversion tracking, multiple form plugin integrations (8 new providers), Release Please-based version automation with GitHub Actions CI/CD workflows, developer tooling (Commitlint, Husky hooks), asset obfuscation for ad-blocker bypass, license system enhancements, and supporting documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Browser
participant Form as Form Plugin
participant PUM_JS as Popup Maker JS
participant Analytics as Analytics Beacon
participant Server as WordPress Server
participant Tracking as FormConversionTracking
participant DB as Database
User->>Form: Submit form
Form->>PUM_JS: Emit form success event
PUM_JS->>PUM_JS: Validate form data
PUM_JS->>PUM_JS: Extract formId, formInstanceId
PUM_JS->>PUM_JS: Get popup context
PUM_JS->>Analytics: Send conversion beacon<br/>(form_submission event)
Analytics->>Server: POST /wp-json/pum/v1/analytics<br/>(eventData + formMetadata)
Server->>Tracking: pum_analytics_conversion<br/>hook triggered
Tracking->>Tracking: Validate form submission type
Tracking->>Tracking: Verify popup exists
Tracking->>DB: UPDATE site count (atomic)
Tracking->>DB: UPDATE popup meta (atomic)
Tracking->>Server: Fire popup_maker/<br/>form_conversion_tracked
Tracking-->>User: Conversion tracked ✓
sequenceDiagram
participant User as User Browser
participant Page as Popup Content
participant LinkHandler as LinkClickTracking JS
participant Analytics as Analytics Beacon
participant Server as WordPress Server
participant Tracking as LinkClickTracking Service
participant DB as Database
User->>Page: Click external/special link
LinkHandler->>LinkHandler: Detect link click
LinkHandler->>LinkHandler: Categorize link type<br/>(external, mailto, tel, etc.)
LinkHandler->>LinkHandler: Build event payload<br/>(link type, popup ID)
LinkHandler->>Analytics: Send beacon
Analytics->>Server: POST /wp-json/pum/v1/analytics<br/>(link_click event)
Server->>Tracking: pum_analytics_conversion<br/>hook triggered
Tracking->>Tracking: Validate link_click type
Tracking->>Tracking: Verify popup exists
Tracking->>DB: UPDATE site count (atomic)
Tracking->>DB: UPDATE popup meta (atomic)
Tracking->>Server: Fire popup_maker/<br/>link_click_tracked
Tracking-->>User: Click tracked ✓
User->>User: Navigate to link
sequenceDiagram
participant Dev as Developer
participant Git as Git Repository
participant GHA as GitHub Actions
participant ReleasePlease as Release Please
participant Slack as Slack Webhook
participant npm as npm Registry
Dev->>Git: Push commits (feat, fix, chore)
Git->>GHA: Trigger on schedule (weekly) or manual
GHA->>ReleasePlease: Run release-please-action
ReleasePlease->>ReleasePlease: Parse commits<br/>(conventional format)
ReleasePlease->>ReleasePlease: Calculate next version
ReleasePlease->>Git: Create/update release PR
Dev->>Git: Merge release PR
ReleasePlease->>Git: Create release tag
ReleasePlease->>GHA: Emit release_created output
GHA->>GHA: Run version:update script
GHA->>Git: Commit version updates
GHA->>Git: Create GitHub Release
GHA->>Slack: Send release notification<br/>(notify-support webhook)
Slack-->>Dev: Notify team ✓
GHA->>npm: Publish if applicable
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/Integration/Form/WPForms.php (1)
76-93:⚠️ Potential issue | 🟡 MinorAdd defensive validation for
form_data['id']in the WPForms hook callback.WPForms documents
$form_dataas an array, but does not explicitly guarantee the presence of an'id'key. Accessing$form_data['id']without validation risks PHP notices and passing unexpected values downstream. Add a defensive check and sanitize withabsint().🔧 Proposed fix
public function on_success( $fields, $entry, $form_data, $entry_id ) { if ( ! $this->should_process_submission() ) { return; } $popup_id = $this->get_popup_id(); + $form_id = ( is_array( $form_data ) && isset( $form_data['id'] ) ) + ? absint( $form_data['id'] ) + : 0; if ( $popup_id ) { $this->increase_conversion( $popup_id ); } pum_integrated_form_submission( [ 'popup_id' => $popup_id, 'form_provider' => $this->key, - 'form_id' => $form_data['id'], + 'form_id' => $form_id, ] ); }Per coding guidelines: use defensive validation in hook callbacks for graceful handling of third-party plugin parameter changes.
🤖 Fix all issues with AI agents
In `@assets/js/src/integration/beaverbuilder.js`:
- Around line 9-66: The ajaxComplete handler assumes settings.data is a string
and calls indexOf, which can throw if settings.data is an object; update the
handler to normalize settings.data to a query-string before using it: detect if
settings.data is an object (typeof settings.data === 'object' or Array.isArray),
convert it to a URLSearchParams instance (reuse the params variable) and then
use params.toString() / params.get(...) everywhere instead of calling indexOf on
settings.data; ensure subsequent uses (the initial action checks, nodeId
extraction via params.get('node_id'), and action variable resolution for
formType) all use the normalized params so the handler won't crash on non-string
data.
In `@assets/js/src/integration/elementor.js`:
- Around line 13-25: The code currently sets $form = $( this )[0] which yields a
DOM element but window.PUM.integrations.formSubmission expects a jQuery object;
change how $form is assigned so it remains a jQuery-wrapped form (e.g., keep
$form as $( this ) instead of [0]) and pass that jQuery object into
window.PUM.integrations.formSubmission along with the existing formProvider and
elementId variables so the call to formSubmission receives the correct type.
In `@assets/js/src/integration/kaliForms.js`:
- Around line 20-82: Both event handlers (the kaliformProcessCompleted listener
and the submit handler that attaches successHandler for kaliFormSuccess) can
call window.PUM.integrations.formSubmission() twice; add a per-form dedupe check
before calling it by marking the form as handled (e.g., set a data attribute
like data-pum-handled or record the element in a WeakSet) in the
kaliformProcessCompleted handler and in successHandler, and skip calling
formSubmission if that flag/WeakSet entry already exists; ensure you set the
flag immediately when invoking window.PUM.integrations.formSubmission() and
remove any temporary listeners (successHandler) as already implemented.
🟡 Minor comments (18)
classes/Controllers/WP/Dashboard.php-75-78 (1)
75-78:⚠️ Potential issue | 🟡 MinorConsider gating widget registration, not just rendering.
The permission check here causes users without
edit_popupspermission to see an empty widget frame with the "Popup Analytics" title but no content. For a cleaner UX, consider adding the same permission check toregister_dashboard_widgets()so the widget isn't registered at all for unauthorized users.Suggested fix in register_dashboard_widgets()
public function register_dashboard_widgets() { + if ( ! current_user_can( $this->container->get_permission( 'edit_popups' ) ) ) { + return; + } + // Basic Analytics Widget (will be moved to free plugin). wp_add_dashboard_widget( 'pum_analytics_basic',If this is applied, you could then remove the permission check from
render_basic_analytics_widget()since the widget wouldn't be registered for unauthorized users.classes/Admin/Popups.php-120-149 (1)
120-149:⚠️ Potential issue | 🟡 MinorAdd defensive validation for
$post_idsin this hook callback.
handle_bulk_actionsassumes$post_idsis a clean array; a defensive cast/sanitize keeps the callback resilient to upstream changes.🛡️ Proposed fix
public static function handle_bulk_actions( $redirect_url, $action, $post_ids ) { if ( ! in_array( $action, [ 'pum_enable', 'pum_disable' ], true ) ) { return $redirect_url; } + if ( ! is_array( $post_ids ) ) { + return $redirect_url; + } + + $post_ids = array_filter( array_map( 'absint', $post_ids ) ); + $enabled = 'pum_enable' === $action ? 1 : 0; $count = 0; $skipped = 0;As per coding guidelines, Use defensive validation in hook callbacks for graceful handling of third-party plugin parameter changes.
classes/DB/Subscribers.php-127-132 (1)
127-132:⚠️ Potential issue | 🟡 MinorAdd
$wpdb->esc_like()to escape table name wildcard characters.The placeholder
%sis correct forSHOW TABLES LIKE(not%i, which is only for SQL identifiers inFROM/WHERE). However, the table name must be escaped with$wpdb->esc_like()to prevent underscores from matching as single-character LIKE wildcards. Without escaping, a table namedwp_pum_subscriberscould unintentionally matchwp_pum_subXcribers.🔧 Suggested fix
- $table_found = $wpdb->get_var( $wpdb->prepare( 'SHOW TABLES LIKE %s', $this->table_name() ) ); + $table_like = $wpdb->esc_like( $this->table_name() ); + $table_found = $wpdb->get_var( $wpdb->prepare( 'SHOW TABLES LIKE %s', $table_like ) );classes/Admin/BlockEditor.php-64-73 (1)
64-73:⚠️ Potential issue | 🟡 MinorAvoid missing-argument warnings for
register_block_assets.Line 68 declares a required
$hook, butenqueue_block_assetsfires with no args, which can emit “missing argument” warnings under WP debug. Make the parameter optional or remove it and update the docblock.🛠️ Proposed fix
- * `@param` string $hook Current page hook. - */ - public static function register_block_assets( $hook ) { + */ + public static function register_block_assets() {classes/ListTable.php-842-846 (1)
842-846:⚠️ Potential issue | 🟡 MinorMake scheme stripping case‑insensitive to fully prevent double‑protocol URLs.
The regex pattern
#^https?://#is case-sensitive and will not match uppercase schemes likeHTTP://orHTTPS://. If a proxy or client sends uppercase schemes, they won't be stripped, resulting in malformed URLs likehttp://HTTP://example.comwhen thehttp://prefix is added. Add theiflag to match schemes regardless of case.🔧 Suggested fix
- $host = preg_replace( '#^https?://#', '', $host ); + $host = preg_replace( '#^https?://#i', '', $host );Also applies to: 1143-1147
classes/Services/LinkClickTracking.php-84-107 (1)
84-107:⚠️ Potential issue | 🟡 MinorPrefer
pum_get_popup()and apum_-prefixed hook for consistency.As per coding guidelines: Use the `pum_get_popup($popup_id)` and `pum_get_theme($theme_id)` functions to get popup and theme objects rather than directly querying post meta; Use the `pum_` or `PUM_` prefix for your functions, classes, and hooks to maintain consistency with the core plugin's standards.🔧 Suggested adjustment
- $popup = get_post( $popup_id ); - if ( ! $popup || 'popup' !== get_post_type( $popup ) ) { + if ( ! function_exists( 'pum_get_popup' ) ) { + return; + } + + $popup = pum_get_popup( $popup_id ); + if ( ! $popup ) { if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) { error_log( sprintf( '[Popup Maker] Skipping link click tracking for invalid popup ID: %d', $popup_id ) ); } return; } @@ - do_action( 'popup_maker/link_click_tracked', $popup_id, $event_data ); + do_action( 'pum_link_click_tracked', $popup_id, $event_data );classes/Integration/Form/FluentForms.php-101-118 (1)
101-118:⚠️ Potential issue | 🟡 MinorKeep form_id extraction compatible with array inputs.
If
$formarrives as an array (some plugin versions/filters),form_idbecomes null and attribution is lost. Add an array fallback.🛠️ Suggested fix
- $form_id = null; - if ( is_object( $form ) && isset( $form->attributes ) && is_object( $form->attributes ) && isset( $form->attributes->id ) ) { - $form_id = $form->attributes->id; - } + $form_id = null; + if ( is_object( $form ) && isset( $form->attributes ) && is_object( $form->attributes ) && isset( $form->attributes->id ) ) { + $form_id = $form->attributes->id; + } elseif ( is_array( $form ) && isset( $form['attributes']['id'] ) ) { + $form_id = $form['attributes']['id']; + }classes/Upsell.php-198-223 (1)
198-223:⚠️ Potential issue | 🟡 MinorUnused variable
$has_eddflagged by static analysis.The variable
$has_eddis declared on Line 200 but never used. It appears the intent was to use it in the conditional logic, but currently only$has_woocommercedetermines the URL.🛠️ Proposed fix: Remove unused variable or use it
If Easy Digital Downloads should have its own specific behavior (which it currently does via the else branch), the variable can be removed:
case 'ecommerce': // Determine which ecommerce platform to link to. $has_woocommerce = in_array( 'WooCommerce', $platforms, true ); - $has_edd = in_array( 'Easy Digital Downloads', $platforms, true ); // Route to specific integration page (WooCommerce preferred if both exist, though rare). if ( $has_woocommerce ) {Alternatively, if you want explicit logic:
- if ( $has_woocommerce ) { + if ( $has_woocommerce && ! $has_edd ) { $base_url = 'https://wppopupmaker.com/ecommerce-integrations/woocommerce/'; + } elseif ( $has_edd && ! $has_woocommerce ) { + $base_url = 'https://wppopupmaker.com/ecommerce-integrations/easy-digital-downloads/'; } else { - $base_url = 'https://wppopupmaker.com/ecommerce-integrations/easy-digital-downloads/'; + // Both detected - default to WooCommerce + $base_url = 'https://wppopupmaker.com/ecommerce-integrations/woocommerce/'; }.github/workflows/request-approval.yml-91-91 (1)
91-91:⚠️ Potential issue | 🟡 MinorMissing quotes around changelog variable assignment.
The changelog output is JSON-escaped by jq, but the variable assignment lacks quotes which could cause word splitting issues with special characters.
🔧 Proposed fix
- CHANGELOG=${{ steps.changelog.outputs.changelog }} + CHANGELOG='${{ steps.changelog.outputs.changelog }}'classes/Controllers/Compatibility/Builder/Divi.php-130-134 (1)
130-134:⚠️ Potential issue | 🟡 MinorDocblock mentions incorrect priority.
The docblock states "This runs at priority 5" but the filter is actually registered at priority 999 (line 63). Update the docblock to reflect the actual behavior.
📝 Proposed fix
/** * Force classic editor for popups when Divi 4 is active. * - * This runs at priority 5 (before PostTypes filter at priority 10) - * to ensure Divi 4's conflicting filters can't override our setting. + * This runs at priority 999 (after Divi 4's priority 100) + * to ensure our setting overrides Divi 4's conflicting filters. *classes/Integration/Form/KaliForms.php-117-127 (1)
117-127:⚠️ Potential issue | 🟡 MinorAdd defensive validation for the Kali Forms hook payload.
The
on_success()method is hooked to a third-party action (kaliforms_after_form_process_action). Without validating the shape of the incoming$argsparameter, accessing nested keys like$args['data']['formId']can cause warnings if the third-party plugin changes its hook signature.🛡️ Suggested fix
public function on_success( $args ) { if ( ! $this->should_process_submission() ) { return; } + if ( ! is_array( $args ) || empty( $args['data'] ) || ! is_array( $args['data'] ) ) { + return; + } + // Get form ID from the data array. $form_id = isset( $args['data']['formId'] ) ? $args['data']['formId'] : null;classes/Integration/Form/KaliForms.php-16-16 (1)
16-16:⚠️ Potential issue | 🟡 MinorNormalize provider key casing for consistency across all integrations.
KaliForms is the only integration using camelCase (
kaliForms); all 18 other integrations use lowercase. Normalizing tokaliformsrequires updates in three locations: the class property, the array key inclasses/Integrations.php, and the JavaScriptformProviderconstant.✏️ Suggested changes
- public $key = 'kaliForms'; + public $key = 'kaliforms';Also update in
classes/Integrations.php:- 'kaliForms' => new PUM_Integration_Form_KaliForms(), + 'kaliforms' => new PUM_Integration_Form_KaliForms(),And in
assets/js/src/integration/kaliForms.js:- const formProvider = 'kaliForms'; + const formProvider = 'kaliforms';classes/Integration/Form/HappyForms.php-123-142 (1)
123-142:⚠️ Potential issue | 🟡 MinorAdd defensive validation for
$formshape in the hook callback.The current
isset( $form['id'] )check can trigger PHP warnings if HappyForms changes the$formparameter type. Guarding the array type ensures graceful handling of third-party plugin changes, following the defensive validation pattern used in other form integrations.🛡️ Suggested fix
- $form_id = isset( $form['id'] ) ? (string) $form['id'] : null; + $form_id = is_array( $form ) && isset( $form['id'] ) ? (string) $form['id'] : null;classes/Integration/Form/Elementor.php-174-194 (1)
174-194:⚠️ Potential issue | 🟡 MinorAdd defensive validation around third-party Elementor handler and address unused parameter.
The
on_success()method is hooked toelementor_pro/forms/new_record, a third-party action. Per the coding guidelines, defensive validation is required for third-party hook callbacks to gracefully handle any future parameter changes. Currently, the code calls$ajax_handler->get_current_form()without verifying the handler object or method exists, and assumes$current_formis an array before using it. Additionally, the$recordparameter is unused and triggers static analysis warnings.Add guards for the
$ajax_handlerobject and method existence, validate$current_formis an array before accessing, and unset the unused$recordparameter:Defensive validation + unused param handling
public function on_success( $record, $ajax_handler ) { if ( ! $this->should_process_submission() ) { return; } + if ( ! is_object( $ajax_handler ) || ! method_exists( $ajax_handler, 'get_current_form' ) ) { + return; + } + // Get element_id to match form selector configuration. $current_form = $ajax_handler->get_current_form(); + if ( ! is_array( $current_form ) ) { + return; + } + + unset( $record ); $element_id = isset( $current_form['id'] ) ? $current_form['id'] : null;classes/Integration/Form/Elementor.php-59-129 (1)
59-129:⚠️ Potential issue | 🟡 MinorGuard against missing
element_idin form results; also consider dependency injection for$wpdb.The query does not filter
WHERE element_id IS NOT NULL, yet the code immediately uses$element_idas an array key without validation. If Elementor submissions include null or emptyelement_idvalues, the entry collapses to an empty key and later entries overwrite each other. Adding a guard clause prevents data loss. Additionally, this method usesglobal $wpdbfor database access; the coding guidelines recommend dependency injection over global access in PHP. Consider refactoring to pass$wpdbvia constructor or method parameter for improved testability.Guard against empty element_id
foreach ( $results as $result ) { $element_id = $result->element_id; + if ( empty( $element_id ) ) { + continue; + } $form_name = $result->form_name;assets/js/src/integration/happyforms.js-8-37 (1)
8-37:⚠️ Potential issue | 🟡 MinorAdd defensive form resolution and guards for edge cases.
The event listener binds to
documentwithout delegating to a specific form selector, soevent.targetcould potentially reference a child element. Using.closest('form.happyforms-form')ensures correct form selection. Additionally, missing guards forformIdand the.index()return value mean invalid submissions could occur if the form ID is absent or the form isn't in the matched set.🛡️ Safer form resolution
- // Extract form element from event target. - const $form = $( event.target ); + // Extract form element from event target. + const $form = $( event.target ).closest( 'form.happyforms-form' ); if ( ! $form.length ) { return; } // Extract form ID from hidden input. const formId = $form.find( '[name="happyforms_form_id"]' ).val(); + if ( ! formId ) { + return; + } // Generate instance ID from form element index (for multiple instances of same form). const $sameIdForms = $( 'form.happyforms-form' ).filter( function () { return ( $( this ).find( '[name="happyforms_form_id"]' ).val() === formId ); } ); - const formInstanceId = $sameIdForms.index( $form ) + 1; + const formInstanceIndex = $sameIdForms.index( $form ); + const formInstanceId = + formInstanceIndex >= 0 ? formInstanceIndex + 1 : null;classes/Integration/Form/BitForm.php-189-206 (1)
189-206:⚠️ Potential issue | 🟡 MinorSilence unused-parameter warnings in
on_success.PHPMD flags
$entry_idand$form_data; explicitly unset (or rename) to avoid CI noise.🧹 Suggested fix
public function on_success( $form_id, $entry_id, $form_data ) { + unset( $entry_id, $form_data ); if ( ! $this->should_process_submission() ) { return; }classes/Integration/Form/BitForm.php-143-169 (1)
143-169:⚠️ Potential issue | 🟡 MinorGuard against invalid form identifiers before querying.
preg_replacecan yield non-numeric values; normalize withabsint()and early-return on0to avoid unnecessary queries and cache keys for invalid IDs.As per coding guidelines, Use WordPress's built-in functions for data validation and sanitization.🔧 Suggested guard
- $numeric_id = is_numeric( $id ) ? $id : preg_replace( '/^(\d+).*/', '$1', $id ); + $numeric_id = is_numeric( $id ) ? $id : preg_replace( '/^(\d+).*/', '$1', $id ); + $numeric_id = absint( $numeric_id ); + + if ( $numeric_id <= 0 ) { + return null; + }
🧹 Nitpick comments (23)
classes/AssetCache.php (4)
1237-1259: Add defensive validation for filter callback parameters.Per coding guidelines, methods hooked to third-party actions/filters should use defensive validation rather than assuming parameter types. The
$tagparameter comes from WordPress and should be validated before regex operations.🛡️ Proposed defensive validation
public static function obfuscate_script_tag( $tag, $handle, $src ) { // Only run on frontend when bypass is enabled. if ( is_admin() || ! pum_get_option( 'bypass_adblockers', false ) ) { return $tag; } + // Defensive validation for third-party filter parameters. + if ( ! is_string( $tag ) || ! is_string( $handle ) ) { + return $tag; + } + // Only obfuscate Popup Maker handles. if ( ! self::should_obfuscate_handle( $handle ) ) { return $tag; }Note: The static analysis warning about unused
$srcis a false positive—this parameter is required by the WordPressscript_loader_tagfilter signature.
1272-1294: Add defensive validation for style tag filter.Similar to the script tag filter, add type validation for the
$tagand$handleparameters received from WordPress.🛡️ Proposed defensive validation
public static function obfuscate_style_tag( $tag, $handle, $href, $media ) { // Only run on frontend when bypass is enabled. if ( is_admin() || ! pum_get_option( 'bypass_adblockers', false ) ) { return $tag; } + // Defensive validation for third-party filter parameters. + if ( ! is_string( $tag ) || ! is_string( $handle ) ) { + return $tag; + } + // Only obfuscate Popup Maker handles. if ( ! self::should_obfuscate_handle( $handle ) ) { return $tag; }Note: The static analysis warnings about unused
$hrefand$mediaare false positives—these parameters are required by the WordPressstyle_loader_tagfilter signature.
1308-1332: Add defensive validation for inline script attributes filter.The
$attributesparameter should be validated as an array before accessing its elements.🛡️ Proposed defensive validation
public static function obfuscate_inline_script_id( $attributes, $data ) { // Only run on frontend when bypass is enabled. if ( is_admin() || ! pum_get_option( 'bypass_adblockers', false ) ) { return $attributes; } + // Defensive validation for third-party filter parameters. + if ( ! is_array( $attributes ) ) { + return $attributes; + } + if ( empty( $attributes['id'] ) ) { return $attributes; }Note: The static analysis warning about unused
$datais a false positive—this parameter is required by the WordPresswp_inline_script_attributesfilter signature. As per coding guidelines, methods hooked to third-party actions/filters should use defensive validation instead of strict types.
1345-1363: Admin handle exclusion may be overly broad.Using
strpos( $handle, 'admin' )excludes any handle containing "admin" anywhere in the string, not just admin-specific scripts. This could inadvertently exclude legitimate frontend handles likepopup-maker-administrator-panelorpum-admin-preview.Consider using a more specific check, such as verifying the handle ends with
-adminor matches known admin handle patterns.♻️ Proposed more precise admin check
foreach ( $patterns as $pattern ) { if ( 0 === strpos( $handle, $pattern ) ) { // Exclude admin scripts. - if ( false !== strpos( $handle, 'admin' ) ) { + if ( preg_match( '/-admin(-|$)/', $handle ) ) { return false; } return true; } }assets/js/src/site/plugins/pum-url-tracking.js (1)
84-102: Consider excluding anchor-only andjavascript:links from tracking.Currently,
shouldTrackClickreturnstruefor anchor links (#section) andjavascript:links. Tracking these as conversions may introduce noise in analytics since they don't represent actual navigation or external engagement.🔧 Proposed fix to exclude non-navigational links
shouldTrackClick: function ( url ) { // Skip empty URLs. if ( ! url ) { return false; } // Skip links already tracked via CTA system. if ( url.indexOf( 'cta=' ) !== -1 ) { return false; } + // Skip anchor-only links (in-page navigation). + if ( url === '#' || ( url.indexOf( '#' ) === 0 && url.indexOf( '#!' ) !== 0 ) ) { + return false; + } + + // Skip javascript: links (not real navigation). + if ( url.indexOf( 'javascript:' ) === 0 ) { + return false; + } + return true; },classes/Admin/BlockEditor.php (1)
75-78: Preferpum_enqueue_scriptfor Popup Maker assets.
popup-maker-block-libraryis a Popup Maker asset; usingpum_enqueue_scriptkeeps it in the asset-cache path. If there’s no specific reason to bypass it, consider switching.♻️ Proposed refactor
- wp_enqueue_script( 'popup-maker-block-library' ); + pum_enqueue_script( 'popup-maker-block-library' );As per coding guidelines: Use the
pum_enqueue_scriptandpum_enqueue_stylefunctions for your assets to allow inclusion in Popup Maker's Asset Cache for optimized performance.classes/Services/LinkClickTracking.php (1)
119-120: Consider injecting$wpdbinstead of using the global.This keeps the service aligned with DI and improves testability.
As per coding guidelines: Use dependency injection over global access in PHP.
classes/Telemetry.php (1)
175-175: TODO identified for form conversion tracking metric.This TODO indicates a planned telemetry enhancement to track form conversions. The referenced option
pum_form_conversion_countshould be implemented alongside the form conversion tracking infrastructure.Would you like me to help implement this metric or open an issue to track this task?
.husky/pre-commit (1)
1-3: Consider deferring this placeholder file until tests are ready.This pre-commit hook contains only comments with no functional logic. Adding placeholder files can create false expectations that pre-commit checks are in place.
Consider either:
- Implementing basic checks (e.g., linting staged files)
- Deferring the file addition until the "future phase" mentioned in the comment
.github/workflows/commitlint.yml (1)
56-65: Cover all pushed commits, not just the last one.On push events,
HEAD~1..HEADonly validates the most recent commit. Use thebefore/afterrange so multi-commit pushes are fully checked.♻️ Suggested change
- else - # For pushes, check the last commit - npx commitlint --from HEAD~1 --to HEAD --verbose + else + # For pushes, check all commits in the push range + npx commitlint --from ${{ github.event.before }} --to ${{ github.event.after }} --verbosebin/extract-changelog.js (1)
47-49: Minor:escapeRegexis missing the hyphen escape.The hyphen (
-) has special meaning inside character classes in regex. While it's not escaped here, it shouldn't cause issues since it's used at the start of the character class[.+*?^$[\](){}|\\]where it would be treated literally anyway. However, for completeness and clarity, consider including it.♻️ Optional: Include hyphen for completeness
function escapeRegex( str ) { - return str.replace( /[.+*?^$[\](){}|\\]/g, '\\$&' ); + return str.replace( /[-/\\^$*+?.()|[\]{}]/g, '\\$&' ); }package.json (1)
174-188: Remove redundant lint commands in lint-staged configuration.The JS/TS and SCSS/CSS lint-staged configurations run
wp-scripts lint-js --fixfollowed bywp-scripts lint-js, which is redundant. The--fixrun already applies fixes and reports any remaining errors; the subsequent run without--fixadds no value.The PHP composer scripts (
composer formatandcomposer lint) do exist incomposer.jsonand are correctly configured.♻️ Suggested simplification
"lint-staged": { "*.php": [ "composer format", "composer lint" ], "*.{js,jsx,ts,tsx}": [ "wp-scripts format", - "wp-scripts lint-js --fix", - "wp-scripts lint-js" + "wp-scripts lint-js --fix" ], "*.{scss,css}": [ "wp-scripts format", - "wp-scripts lint-style --fix", - "wp-scripts lint-style" + "wp-scripts lint-style --fix" ] }.github/workflows/notify-support.yml (1)
197-201: Add error handling for curl request.The curl command doesn't check for HTTP errors. If the webhook URL is invalid or returns an error, the workflow will still report success.
🔧 Proposed fix
run: | echo "📤 Sending support notification to Slack..." - curl -X POST "$SLACK_WEBHOOK_URL" \ + HTTP_CODE=$(curl -s -o /tmp/response.txt -w "%{http_code}" -X POST "$SLACK_WEBHOOK_URL" \ -H "Content-Type: application/json" \ - -d '${{ steps.payload.outputs.payload }}' + -d '${{ steps.payload.outputs.payload }}') + if [ "$HTTP_CODE" -lt 200 ] || [ "$HTTP_CODE" -ge 300 ]; then + echo "❌ Slack request failed with HTTP $HTTP_CODE" + cat /tmp/response.txt + exit 1 + fi echo "" echo "✅ Slack message sent to `#support-updates`!".github/workflows/request-approval.yml (1)
185-194: Add error handling for curl request.Same issue as notify-support.yml - the curl command doesn't verify HTTP response status. Consider adding
-fflag or explicit status checking.🔧 Proposed fix
run: | echo "📤 Sending approval request to Slack..." - curl -X POST "$SLACK_WEBHOOK_URL" \ + HTTP_CODE=$(curl -s -o /tmp/response.txt -w "%{http_code}" -X POST "$SLACK_WEBHOOK_URL" \ -H "Content-Type: application/json" \ - -d '${{ steps.payload.outputs.payload }}' + -d '${{ steps.payload.outputs.payload }}') + if [ "$HTTP_CODE" -lt 200 ] || [ "$HTTP_CODE" -ge 300 ]; then + echo "❌ Slack request failed with HTTP $HTTP_CODE" + cat /tmp/response.txt + exit 1 + fi echo "" echo "✅ Slack message sent!".github/workflows/ci.yml (1)
7-8: Unused environment variable.
WP_VERSIONis defined but never used in the workflow. Consider removing it or utilizing it in the PHPUnit job when enabled.🧹 Proposed fix
-env: - WP_VERSION: 6.1.1 -Or keep it for when PHPUnit is enabled and reference it in the install step:
- name: Install WordPress test environment - run: bash bin/install-wp-tests.sh wordpress_test root 'password' mysql + run: bash bin/install-wp-tests.sh wordpress_test root 'password' mysql ${{ env.WP_VERSION }}commitlint.config.js (1)
39-42: Consider allowing empty scopes for simple commits.The
scope-enumrule is set to level 2 (error) with'always', which requires a scope on every commit. This might be overly restrictive for simple maintenance commits. Consider using level 1 (warn) or adding an empty string to allow scope-less commits.🔧 Option to allow empty scopes
'scope-enum': [ - 2, + 1, 'always', [Or keep level 2 but make scope optional by changing to
'scope-case'rule only, removingscope-enumenforcement.classes/Integration/Form/HTMLForms.php (2)
1-8: Consider adding namespace declaration.Per coding guidelines, new modern integrations should use the
PopupMaker\namespace. This class lacks a namespace declaration while other integration classes in the PR use namespaces.🔧 Proposed fix
<?php /** * Integration for HTML Forms * * `@package` PopupMaker * `@copyright` Copyright (c) 2024, Code Atlantic LLC */ + +namespace PopupMaker\Integration\Form; + +use PUM_Abstract_Integration_Form; + class PUM_Integration_Form_HTMLForms extends PUM_Abstract_Integration_Form {Note: This would require updating the class registration to use the fully qualified name.
As per coding guidelines: "Use the
PopupMaker\namespace for new, modern integrations where appropriate, following the structure seen in theclasses/directory."
95-99: Exception handling may be unnecessary.The
hf_get_form()function from HTML Forms typically returnsnullfor invalid IDs rather than throwing an exception. The try-catch may be defensive overkill, though it doesn't cause harm.🔧 Simplified version
public function get_form( $id ) { if ( ! $this->enabled() ) { return null; } - try { - return hf_get_form( $id ); - } catch ( Exception $e ) { - return null; - } + return hf_get_form( $id ); }assets/js/src/integration/newsletter.js (1)
63-65: Remove redundantwindow.PUMguards to avoid dead branches.These integration modules are bundled with
window.PUM, so the extra guards are likely unreachable and add noise.♻️ Suggested simplification
- if ( ! window.PUM || ! window.PUM.integrations ) { - return; - } - window.PUM.integrations.formSubmission( $( container ), { formProvider, formId: null, @@ - if ( ! window.PUM ) { - return; - } - // Observe existing forms. initObservers();Based on learnings: “In assets/js/src/integration/**/*.js, guards that check for window.PUM (e.g., conditional blocks that run only if window.PUM exists) are redundant because these integration files are bundled with window.PUM. Remove such guards and any branches dependent on the presence of window.PUM to simplify code and avoid dead branches.”
Also applies to: 176-180
assets/js/src/integration/htmlforms.js (1)
20-23: Consider logging when formId falls back to 'unknown'.Silently defaulting to
'unknown'may make it harder to debug trigger issues. Consider adding a console warning in debug mode when the fallback is used.classes/Integration/Form/BeaverBuilder.php (1)
47-62: Clarify the mock form approach in documentation.The static form list (
contact_any,subscribe_any,login_any) is a pragmatic workaround since Beaver Builder forms are page instances rather than centrally registered entities. Consider adding a brief class-level docblock explaining this design choice for future maintainers.📝 Suggested documentation addition
/** * Beaver Builder Forms Integration Class + * + * Note: Beaver Builder forms are per-page instances, not centrally registered. + * This integration provides generic "Any X Form" options for triggering popups + * on any BB form of a given type. Actual form tracking happens via JavaScript. */ class PUM_Integration_Form_BeaverBuilder extends PUM_Abstract_Integration_Form {classes/Integration/Form/BitForm.php (2)
9-31: Consider namespacing this new integration class.Moving it under a
PopupMaker\…namespace reduces global symbol collisions and aligns with modern structure inclasses/.As per coding guidelines, Use custom
\PopupMaker\{ExtensionName}\namespace for classes/functions/hooks in PHP.
89-98: Prefer injecting$wpdbinstead of using the global.This improves testability and reduces reliance on global state in both data access methods.
As per coding guidelines, Use dependency injection over global access in PHP.
Also applies to: 154-163
| // Hook into jQuery AJAX complete for all Beaver Builder forms. | ||
| $( document ).on( 'ajaxComplete', function ( _event, xhr, settings ) { | ||
| // Check if this is a Beaver Builder form submission. | ||
| if ( | ||
| ! settings.data || | ||
| ( settings.data.indexOf( 'action=fl_builder_email' ) === -1 && | ||
| settings.data.indexOf( | ||
| 'action=fl_builder_subscribe_form_submit' | ||
| ) === -1 ) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| let response; | ||
| try { | ||
| response = | ||
| typeof xhr.responseJSON !== 'undefined' | ||
| ? xhr.responseJSON | ||
| : JSON.parse( xhr.responseText ); | ||
| } catch ( e ) { | ||
| return; // Not JSON response. | ||
| } | ||
|
|
||
| // Check for success (both forms use response.data). | ||
| const data = response.data || {}; | ||
|
|
||
| // Contact form: data.error === false | ||
| // Subscribe form: !data.error | ||
| if ( data.error !== false && data.error ) { | ||
| return; // Form had errors. | ||
| } | ||
|
|
||
| // Extract form type and node ID from AJAX data. | ||
| const params = new URLSearchParams( settings.data ); | ||
| const nodeId = params.get( 'node_id' ); | ||
|
|
||
| if ( ! nodeId ) { | ||
| return; | ||
| } | ||
|
|
||
| // Find the form element. | ||
| const $module = $( '.fl-node-' + nodeId ); | ||
| const $form = $module | ||
| .find( '.fl-contact-form, .fl-subscribe-form' ) | ||
| .first(); | ||
|
|
||
| if ( ! $form.length ) { | ||
| return; | ||
| } | ||
|
|
||
| // Determine form type from action. | ||
| const action = params.get( 'action' ); | ||
| let formType = 'unknown'; | ||
| if ( action === 'fl_builder_email' ) { | ||
| formType = 'contact'; | ||
| } else if ( action === 'fl_builder_subscribe_form_submit' ) { | ||
| formType = 'subscribe'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "beaverbuilder.js" --type fileRepository: PopupMaker/Popup-Maker
Length of output: 108
🏁 Script executed:
cat -n assets/js/src/integration/beaverbuilder.jsRepository: PopupMaker/Popup-Maker
Length of output: 2760
🌐 Web query:
jQuery AJAX settings.data object vs string type behavior
💡 Result:
In $.ajax({ data: ... }), jQuery treats object/array vs string differently:
-
If
datais an object/array: jQuery will serialize it into a URL-encoded query string (via$.param) as long asprocessData: true(the default). Example:{ a: "bc", d: "e,f" }→"a=bc&d=e%2Cf". Arrays are serialized according totraditional. For GET (and other “no request body” methods), this serialized string is appended to the URL. [1] -
If
datais a string: jQuery assumes it is already encoded appropriately for the request and does not re-encode/serialize it; it’s sent “as-is” (or appended to the URL for GET). [1]
Key settings that control the behavior:
-
processData(defaulttrue): controls whether non-stringdatagets converted to a query string. SetprocessData: falsewhen sending non-URL-encoded bodies (e.g.,FormData, raw JSON string). [1] -
contentType(default"application/x-www-form-urlencoded; charset=UTF-8"): describes the body format you’re sending; if you send JSON, you typically setcontentType: "application/json"and passdata: JSON.stringify(obj)(a string) so jQuery won’t URL-encode it. [1]
Sources: jQuery $.ajax() docs on data, processData, and contentType. [1]
Handle non-string settings.data to prevent ajaxComplete handler crashes.
settings.data can be an object for some jQuery AJAX requests; calling .indexOf() will throw TypeError and crash the handler for unrelated AJAX calls. Normalize data to a query string once via URLSearchParams with type checking, then reuse it.
🛠️ Suggested fix
- // Check if this is a Beaver Builder form submission.
- if (
- ! settings.data ||
- ( settings.data.indexOf( 'action=fl_builder_email' ) === -1 &&
- settings.data.indexOf(
- 'action=fl_builder_subscribe_form_submit'
- ) === -1 )
- ) {
- return;
- }
+ if ( ! settings.data ) {
+ return;
+ }
+
+ const params = new URLSearchParams(
+ typeof settings.data === 'string' ? settings.data : $.param( settings.data )
+ );
+ const action = params.get( 'action' );
+ if (
+ action !== 'fl_builder_email' &&
+ action !== 'fl_builder_subscribe_form_submit'
+ ) {
+ return;
+ }
@@
- const params = new URLSearchParams( settings.data );
- const nodeId = params.get( 'node_id' );
+ const nodeId = params.get( 'node_id' );
@@
- const action = params.get( 'action' );
let formType = 'unknown';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Hook into jQuery AJAX complete for all Beaver Builder forms. | |
| $( document ).on( 'ajaxComplete', function ( _event, xhr, settings ) { | |
| // Check if this is a Beaver Builder form submission. | |
| if ( | |
| ! settings.data || | |
| ( settings.data.indexOf( 'action=fl_builder_email' ) === -1 && | |
| settings.data.indexOf( | |
| 'action=fl_builder_subscribe_form_submit' | |
| ) === -1 ) | |
| ) { | |
| return; | |
| } | |
| let response; | |
| try { | |
| response = | |
| typeof xhr.responseJSON !== 'undefined' | |
| ? xhr.responseJSON | |
| : JSON.parse( xhr.responseText ); | |
| } catch ( e ) { | |
| return; // Not JSON response. | |
| } | |
| // Check for success (both forms use response.data). | |
| const data = response.data || {}; | |
| // Contact form: data.error === false | |
| // Subscribe form: !data.error | |
| if ( data.error !== false && data.error ) { | |
| return; // Form had errors. | |
| } | |
| // Extract form type and node ID from AJAX data. | |
| const params = new URLSearchParams( settings.data ); | |
| const nodeId = params.get( 'node_id' ); | |
| if ( ! nodeId ) { | |
| return; | |
| } | |
| // Find the form element. | |
| const $module = $( '.fl-node-' + nodeId ); | |
| const $form = $module | |
| .find( '.fl-contact-form, .fl-subscribe-form' ) | |
| .first(); | |
| if ( ! $form.length ) { | |
| return; | |
| } | |
| // Determine form type from action. | |
| const action = params.get( 'action' ); | |
| let formType = 'unknown'; | |
| if ( action === 'fl_builder_email' ) { | |
| formType = 'contact'; | |
| } else if ( action === 'fl_builder_subscribe_form_submit' ) { | |
| formType = 'subscribe'; | |
| } | |
| // Hook into jQuery AJAX complete for all Beaver Builder forms. | |
| $( document ).on( 'ajaxComplete', function ( _event, xhr, settings ) { | |
| if ( ! settings.data ) { | |
| return; | |
| } | |
| const params = new URLSearchParams( | |
| typeof settings.data === 'string' ? settings.data : $.param( settings.data ) | |
| ); | |
| const action = params.get( 'action' ); | |
| if ( | |
| action !== 'fl_builder_email' && | |
| action !== 'fl_builder_subscribe_form_submit' | |
| ) { | |
| return; | |
| } | |
| let response; | |
| try { | |
| response = | |
| typeof xhr.responseJSON !== 'undefined' | |
| ? xhr.responseJSON | |
| : JSON.parse( xhr.responseText ); | |
| } catch ( e ) { | |
| return; // Not JSON response. | |
| } | |
| // Check for success (both forms use response.data). | |
| const data = response.data || {}; | |
| // Contact form: data.error === false | |
| // Subscribe form: !data.error | |
| if ( data.error !== false && data.error ) { | |
| return; // Form had errors. | |
| } | |
| // Extract form type and node ID from AJAX data. | |
| const nodeId = params.get( 'node_id' ); | |
| if ( ! nodeId ) { | |
| return; | |
| } | |
| // Find the form element. | |
| const $module = $( '.fl-node-' + nodeId ); | |
| const $form = $module | |
| .find( '.fl-contact-form, .fl-subscribe-form' ) | |
| .first(); | |
| if ( ! $form.length ) { | |
| return; | |
| } | |
| // Determine form type from action. | |
| let formType = 'unknown'; | |
| if ( action === 'fl_builder_email' ) { | |
| formType = 'contact'; | |
| } else if ( action === 'fl_builder_subscribe_form_submit' ) { | |
| formType = 'subscribe'; | |
| } |
🤖 Prompt for AI Agents
In `@assets/js/src/integration/beaverbuilder.js` around lines 9 - 66, The
ajaxComplete handler assumes settings.data is a string and calls indexOf, which
can throw if settings.data is an object; update the handler to normalize
settings.data to a query-string before using it: detect if settings.data is an
object (typeof settings.data === 'object' or Array.isArray), convert it to a
URLSearchParams instance (reuse the params variable) and then use
params.toString() / params.get(...) everywhere instead of calling indexOf on
settings.data; ensure subsequent uses (the initial action checks, nodeId
extraction via params.get('node_id'), and action variable resolution for
formType) all use the normalized params so the handler won't crash on non-string
data.
| const $form = $( this )[ 0 ]; | ||
|
|
||
| // Get element_id from the widget container. | ||
| // Elementor form widgets are inside a .elementor-element-{id} container. | ||
| const $widget = $( this ).closest( '[data-id]' ); | ||
| const elementId = $widget.length | ||
| ? $widget.attr( 'data-id' ) | ||
| : 'unknown'; | ||
|
|
||
| window.PUM.integrations.formSubmission( $form, { | ||
| formProvider, | ||
| formId: elementId, | ||
| } ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n assets/js/src/integration/elementor.jsRepository: PopupMaker/Popup-Maker
Length of output: 1062
🏁 Script executed:
# Search for formSubmission implementation to understand what it expects
rg -A 10 "formSubmission\s*\(" --type js -B 2Repository: PopupMaker/Popup-Maker
Length of output: 15286
🏁 Script executed:
# Check other integration files to see the pattern they use
ls -la assets/js/src/integration/Repository: PopupMaker/Popup-Maker
Length of output: 1370
🏁 Script executed:
# Look for formSubmission in the main codebase
rg "\.formSubmission" --type js -B 2 -A 5Repository: PopupMaker/Popup-Maker
Length of output: 10348
🏁 Script executed:
# Find the formSubmission function implementation
rg -A 20 "formSubmission\s*\(\s*\w+\s*," assets/js/src/site/plugins/pum-integrations.jsRepository: PopupMaker/Popup-Maker
Length of output: 750
🏁 Script executed:
# Look for the full definition/implementation of formSubmission
ast-grep --pattern $'integrations: {
$$$
formSubmission($_) {
$$$
}
$$$
}'Repository: PopupMaker/Popup-Maker
Length of output: 48
🏁 Script executed:
# Check where formSubmission is defined as a method
rg "formSubmission\s*:" --type js -B 5 -A 15Repository: PopupMaker/Popup-Maker
Length of output: 1631
🏁 Script executed:
# Get more context on the formSubmission implementation
rg -A 30 "formSubmission: function" assets/js/src/site/plugins/pum-integrations.jsRepository: PopupMaker/Popup-Maker
Length of output: 885
🏁 Script executed:
# Check the getPopup function to see how it handles form parameter
rg -B 5 -A 15 "getPopup\s*:\s*function|getPopup\s*\(" assets/js/src/site/plugins/ --type jsRepository: PopupMaker/Popup-Maker
Length of output: 50378
Pass a jQuery object to formSubmission, not a DOM element.
Line 13 passes $( this )[ 0 ] (a DOM node). According to the API, window.PUM.integrations.formSubmission() expects a jQuery-wrapped form object.
Fix
- const $form = $( this )[ 0 ];
+ const $form = $( this );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const $form = $( this )[ 0 ]; | |
| // Get element_id from the widget container. | |
| // Elementor form widgets are inside a .elementor-element-{id} container. | |
| const $widget = $( this ).closest( '[data-id]' ); | |
| const elementId = $widget.length | |
| ? $widget.attr( 'data-id' ) | |
| : 'unknown'; | |
| window.PUM.integrations.formSubmission( $form, { | |
| formProvider, | |
| formId: elementId, | |
| } ); | |
| const $form = $( this ); | |
| // Get element_id from the widget container. | |
| // Elementor form widgets are inside a .elementor-element-{id} container. | |
| const $widget = $( this ).closest( '[data-id]' ); | |
| const elementId = $widget.length | |
| ? $widget.attr( 'data-id' ) | |
| : 'unknown'; | |
| window.PUM.integrations.formSubmission( $form, { | |
| formProvider, | |
| formId: elementId, | |
| } ); |
🤖 Prompt for AI Agents
In `@assets/js/src/integration/elementor.js` around lines 13 - 25, The code
currently sets $form = $( this )[0] which yields a DOM element but
window.PUM.integrations.formSubmission expects a jQuery object; change how $form
is assigned so it remains a jQuery-wrapped form (e.g., keep $form as $( this )
instead of [0]) and pass that jQuery object into
window.PUM.integrations.formSubmission along with the existing formProvider and
elementId variables so the call to formSubmission receives the correct type.
| document.addEventListener( 'kaliformProcessCompleted', function ( event ) { | ||
| const detail = event.detail; | ||
|
|
||
| if ( ! detail || ! detail.form ) { | ||
| return; | ||
| } | ||
|
|
||
| const $form = $( detail.form ); | ||
| const formId = $form.data( 'form-id' ); | ||
| const formInstanceId = $form.attr( 'id' ); // e.g., "kaliforms-form-123" | ||
|
|
||
| // All the magic happens here. | ||
| window.PUM.integrations.formSubmission( $form, { | ||
| formProvider, | ||
| formId, | ||
| formInstanceId, | ||
| } ); | ||
| } ); | ||
|
|
||
| /** | ||
| * Alternative approach: Listen for successful AJAX response. | ||
| * Kali Forms processes forms via AJAX action 'kaliforms_form_process'. | ||
| */ | ||
| $( document ).on( 'submit', 'form[data-form-id]', function ( event ) { | ||
| const $form = $( this ); | ||
| const formClass = $form.attr( 'class' ); | ||
|
|
||
| // Check if this is a Kali Forms form. | ||
| if ( ! formClass || ! formClass.includes( 'kali-form' ) ) { | ||
| return; | ||
| } | ||
|
|
||
| // Store reference for success handler. | ||
| const formId = $form.data( 'form-id' ); | ||
| const formInstanceId = $form.attr( 'id' ); | ||
|
|
||
| // Listen for successful form processing via custom event. | ||
| const successHandler = function ( successEvent ) { | ||
| if ( | ||
| successEvent.detail && | ||
| successEvent.detail.formId === formId | ||
| ) { | ||
| window.PUM.integrations.formSubmission( $form, { | ||
| formProvider, | ||
| formId, | ||
| formInstanceId, | ||
| } ); | ||
|
|
||
| // Remove the listener after firing once. | ||
| document.removeEventListener( | ||
| 'kaliFormSuccess', | ||
| successHandler | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener( 'kaliFormSuccess', successHandler ); | ||
|
|
||
| // Cleanup listener after 30 seconds if not fired. | ||
| setTimeout( function () { | ||
| document.removeEventListener( 'kaliFormSuccess', successHandler ); | ||
| }, 30000 ); | ||
| } ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the complete kaliForms.js file to understand context
cat -n assets/js/src/integration/kaliForms.jsRepository: PopupMaker/Popup-Maker
Length of output: 4214
🌐 Web query:
Kali Forms AJAX events kaliformProcessCompleted kaliFormSuccess documentation
💡 Result:
I couldn’t find any official Kali Forms documentation for AJAX lifecycle events named kaliformProcessCompleted (note the lowercase “f”) or kaliFormSuccess.
What is publicly shown in Kali Forms’ own docs/blog examples is a frontend CustomEvent you can reliably hook into:
kaliFormProcessConstructed— used as the “form is initialized/constructed” hook to run custom JavaScript (often to start your own AJAX, prefill fields, disable options, etc.).[1][2]
Example (from their usage pattern):
document.addEventListener('kaliFormProcessConstructed', function (event) {
// your code (e.g., attach submit handlers, run AJAX, etc.)
});If you specifically need “submission success/completed” hooks, you’ll likely have to inspect the plugin’s frontend JS bundle to see which events it actually dispatches on submit (and their exact names), because those two event names don’t appear in their published docs/examples.[1][2]
Sources: [1] [2]
🏁 Script executed:
# Also check if there are any other files or comments that might clarify the event flow
rg -i "kaliForms|kaliforms" --type js -B 2 -A 2 | head -100Repository: PopupMaker/Popup-Maker
Length of output: 2812
🏁 Script executed:
# Check for test files related to kaliForms
fd -t f ".*kali.*test|.*test.*kali" -i
# Check git history for context on this file
git log --oneline -20 -- assets/js/src/integration/kaliForms.js
# Look for any comments or documentation about event selection
rg -i "kaliformProcessCompleted|kaliFormSuccess" -B 5 -A 5Repository: PopupMaker/Popup-Maker
Length of output: 2216
🏁 Script executed:
# Check other integration files for similar patterns to understand the design
ls -la assets/js/src/integration/
head -80 assets/js/src/integration/htmlforms.js | cat -nRepository: PopupMaker/Popup-Maker
Length of output: 2940
Guard against double-tracking when both Kali Forms events fire.
The two event listeners (kaliformProcessCompleted and kaliFormSuccess) can both trigger window.PUM.integrations.formSubmission() for the same form submission without deduplication. If Kali Forms emits both events (or does so in future versions), conversions will be counted twice. Add a per-form handled flag or consolidate to a single reliable event.
🛠️ Possible dedupe approach
const formProvider = 'kaliForms';
const $ = window.jQuery;
+
+ const submitOnce = ( $form, formId, formInstanceId ) => {
+ if ( $form.data( 'pumKaliFormsHandled' ) ) {
+ return;
+ }
+ $form.data( 'pumKaliFormsHandled', true );
+ window.PUM.integrations.formSubmission( $form, {
+ formProvider,
+ formId,
+ formInstanceId,
+ } );
+ };
@@
- // All the magic happens here.
- window.PUM.integrations.formSubmission( $form, {
- formProvider,
- formId,
- formInstanceId,
- } );
+ // All the magic happens here.
+ submitOnce( $form, formId, formInstanceId );
@@
- window.PUM.integrations.formSubmission( $form, {
- formProvider,
- formId,
- formInstanceId,
- } );
+ submitOnce( $form, formId, formInstanceId );🤖 Prompt for AI Agents
In `@assets/js/src/integration/kaliForms.js` around lines 20 - 82, Both event
handlers (the kaliformProcessCompleted listener and the submit handler that
attaches successHandler for kaliFormSuccess) can call
window.PUM.integrations.formSubmission() twice; add a per-form dedupe check
before calling it by marking the form as handled (e.g., set a data attribute
like data-pum-handled or record the element in a WeakSet) in the
kaliformProcessCompleted handler and in successHandler, and skip calling
formSubmission if that flag/WeakSet entry already exists; ensure you set the
flag immediately when invoking window.PUM.integrations.formSubmission() and
remove any temporary listeners (successHandler) as already implemented.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
❌ Commit Validation ErrorYour commits don't pass validation. Common issues: Format RequirementsCommits must follow conventional commit format: Valid types: feat, fix, improve, perf, refactor, docs, style, test, build, ci, chore, revert Examples:
|
2 participants
Description
Problem
CTA button styles we see using the block editor aren't applying to the
[pum_cta]shortcode. So instead of a button, it only shows as a plain hypertext link.On top of that, when we just try to apply the CTA button block styles, to the classic editor CTA button, we get the following:
Summary
[pum_cta]shortcode outputTest plan
[pum_cta]shortcode - verify rounded corners and proper width[pum_cta]shortcode - verify rounded corners and auto-width (not full-width)Related Issue:
Types of changes
Screenshots
This has been tested in the following browsers
Merge Checklist
Summary by CodeRabbit
New Features
Improvements