feat(ui): add popup title panel to Block Editor document settings#1160
feat(ui): add popup title panel to Block Editor document settings#1160
Conversation
Add a new Popup Title panel to the Block Editor sidebar that allows users to set the popup title and toggle whether it displays on the frontend. Changes: - Add popup_title REST field with update_callback in RestAPI.php - Create popup-title-panel.tsx component with title input and display toggle - Use module-level storage for pending values with save-on-publish pattern - Subscribe to Block Editor save events to persist custom fields via REST API
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds popup title storage and editing: a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Editor as Gutenberg Editor
participant Panel as PopupTitlePanel
participant Store as WP Data Store
participant API as REST API (/popup-maker/v2)
participant DB as Post Meta
User->>Panel: Edit title / toggle display
Panel->>Panel: Update local state & pendingValues
User->>Editor: Save post
Editor->>Panel: Invoke save subscription
Panel->>Panel: Check hasChanges / previous save state
alt pending changes exist
Panel->>API: PATCH /popups/{id} (popup_title + popup_settings)
API->>DB: Persist meta (popup_title, popup_settings)
DB-->>API: Success
API-->>Panel: Response
Panel->>Panel: Clear pendingValues, reset flags
Panel->>Store: Update editor store (optional)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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: 2
🤖 Fix all issues with AI agents
In `@classes/Controllers/RestAPI.php`:
- Around line 357-373: Replace direct post meta access in the
register_rest_field call for 'popup_title' with the popup object methods: in the
'get_callback' use pum_get_popup($obj->ID) and return $popup ?
$popup->get_title() : ''; in the 'update_callback' retrieve the popup via
pum_get_popup($obj->ID) and call $popup->update_meta('popup_title',
sanitize_text_field($value)) only if $popup exists; keep schema and
permission_callback unchanged and mirror the pattern used by the existing
'popup_settings' handler.
In `@packages/block-editor/src/plugins/popup-title-panel.tsx`:
- Around line 93-99: The current .catch handler in the Promise chain inside
popup-title-panel.tsx (the handler that logs "Failed to save popup title
settings") only writes to console and leaves users unaware of save failures;
update the catch for the save call to also create a user-visible error notice
via the WordPress notices store (dispatch('core/notices').createErrorNotice or
the Popup Maker notification API) with a localized message, keep the
console.error for debugging, and ensure any UI state like
pendingValues.hasChanges remains accurate so the user can retry.
🧹 Nitpick comments (2)
packages/block-editor/src/plugins/popup-title-panel.tsx (2)
8-8: Consider using@popup-maker/i18nfor consistency.Per coding guidelines, prioritize
@popup-maker/*packages over@wordpress/*when available. The@popup-maker/i18npackage exports a__function (seepackages/i18n/src/index.ts).Suggested change
-import { __ } from '@wordpress/i18n'; +import { __ } from '@popup-maker/i18n';
16-19: LocalPopupSettingstype may conflict with@popup-maker/core-data.A
PopupSettingstype exists in@popup-maker/core-data. Consider importing and extending it, or renaming this local interface to avoid shadowing (e.g.,LocalPopupSettingsorPopupTitleSettings).
| // Register popup_title as REST field with explicit update_callback. | ||
| // This ensures saves work correctly with custom REST namespace. | ||
| register_rest_field( $post_type, 'popup_title', [ | ||
| 'get_callback' => function ( $obj ) { | ||
| return get_post_meta( $obj['id'], 'popup_title', true ) ?: ''; | ||
| }, | ||
| 'update_callback' => function ( $value, $obj ) { | ||
| update_post_meta( $obj->ID, 'popup_title', sanitize_text_field( $value ) ); | ||
| }, | ||
| 'schema' => [ | ||
| 'type' => 'string', | ||
| 'description' => __( 'The popup title displayed inside the popup.', 'popup-maker' ), | ||
| ], | ||
| 'permission_callback' => function () use ( $edit_permission ) { | ||
| return current_user_can( $edit_permission ); | ||
| }, | ||
| ] ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all popup_title references with context
rg -n "popup_title" --type=php -B2 -A2Repository: PopupMaker/Popup-Maker
Length of output: 9973
🏁 Script executed:
#!/bin/bash
# Check the PUM_Model_Popup class for available methods
fd "class.*Popup" --type=php | head -20Repository: PopupMaker/Popup-Maker
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Look at the 'enabled' field pattern mentioned in the comment
sed -n '346,350p' classes/Controllers/RestAPI.phpRepository: PopupMaker/Popup-Maker
Length of output: 285
🏁 Script executed:
#!/bin/bash
# Check for get_setting/update_setting patterns in RestAPI.php
rg -n "get_setting|update_setting" classes/Controllers/RestAPI.php -B1 -A1Repository: PopupMaker/Popup-Maker
Length of output: 275
Use the popup object methods for getting and updating popup_title in REST callbacks.
Per coding guidelines, use pum_get_popup() to retrieve the popup object, then call get_title() or update_meta() instead of direct get_post_meta()/update_post_meta() calls. The same file demonstrates this pattern in the popup_settings field handler (lines 390–391).
Suggested pattern (from lines 390–391):
'get_callback' => function ( $obj ) {
$popup = pum_get_popup( $obj->ID );
return $popup ? $popup->get_title() : '';
},
'update_callback' => function ( $value, $obj ) {
$popup = pum_get_popup( $obj->ID );
if ( $popup ) {
$popup->update_meta( 'popup_title', sanitize_text_field( $value ) );
}
},🤖 Prompt for AI Agents
In `@classes/Controllers/RestAPI.php` around lines 357 - 373, Replace direct post
meta access in the register_rest_field call for 'popup_title' with the popup
object methods: in the 'get_callback' use pum_get_popup($obj->ID) and return
$popup ? $popup->get_title() : ''; in the 'update_callback' retrieve the popup
via pum_get_popup($obj->ID) and call $popup->update_meta('popup_title',
sanitize_text_field($value)) only if $popup exists; keep schema and
permission_callback unchanged and mirror the pattern used by the existing
'popup_settings' handler.
| .then( () => { | ||
| pendingValues.hasChanges = false; | ||
| } ) | ||
| .catch( ( error ) => { | ||
| // eslint-disable-next-line no-console | ||
| console.error( 'Failed to save popup title settings:', error ); | ||
| } ); |
There was a problem hiding this comment.
Silent failure may confuse users when API save fails.
The .catch() only logs to console. If the REST API call fails (network issue, permission error, etc.), the user sees no indication that their popup title changes weren't saved. Consider displaying a notice using @wordpress/notices or the Popup Maker notification system.
Example improvement
import { dispatch } from '@wordpress/data';
// In the .catch handler:
.catch( ( error ) => {
console.error( 'Failed to save popup title settings:', error );
dispatch( 'core/notices' ).createErrorNotice(
__( 'Failed to save popup title settings.', 'popup-maker' ),
{ type: 'snackbar' }
);
} );🤖 Prompt for AI Agents
In `@packages/block-editor/src/plugins/popup-title-panel.tsx` around lines 93 -
99, The current .catch handler in the Promise chain inside popup-title-panel.tsx
(the handler that logs "Failed to save popup title settings") only writes to
console and leaves users unaware of save failures; update the catch for the save
call to also create a user-visible error notice via the WordPress notices store
(dispatch('core/notices').createErrorNotice or the Popup Maker notification API)
with a localized message, keep the console.error for debugging, and ensure any
UI state like pendingValues.hasChanges remains accurate so the user can retry.
- Add display_title setting to show/hide popup title on frontend - Register popup_title and popup_settings meta for REST API support - Update popup template with screen-reader fallback for accessibility - Add display title checkbox toggle in classic editor - Style the toggle with proper alignment and hover states
❌ 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:
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
classes/Controllers/PostTypes.php (3)
171-173: Auth callbacks use genericedit_postsinstead of popup-specific capability.The popup post type registration (lines 135-139) uses
$this->container->get_permission( 'edit_popups' ), but these meta auth callbacks use the genericedit_postscapability. Consider using the same permission for consistency.Also applies to: 190-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/Controllers/PostTypes.php` around lines 171 - 173, The meta box auth callbacks currently call current_user_can('edit_posts') which is inconsistent with the popup post type permission; replace those auth_callback closures to check the popup-specific capability by calling current_user_can($this->container->get_permission('edit_popups')) (or otherwise use $this->container->get_permission('edit_popups') inside the closure) so the same permission used in the popup registration is enforced; update both occurrences (the closure at the shown diff and the similar one around 190-192).
220-222: Nested arrays pass through without sanitization.The comment indicates this is for
conditions,triggers, andcookies, but the code allows any nested array to bypass sanitization. This could be a security concern if unexpected nested data is submitted via the REST API.Consider applying recursive sanitization or explicitly validating that only known nested keys are allowed to pass through.
🔧 Proposed defensive approach
} elseif ( is_array( $setting ) ) { - // Nested arrays (conditions, triggers, cookies) pass through. - $sanitized[ $key ] = $setting; + // Only allow known complex setting keys to pass through. + $allowed_nested_keys = [ 'conditions', 'triggers', 'cookies' ]; + if ( in_array( $key, $allowed_nested_keys, true ) ) { + $sanitized[ $key ] = $setting; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/Controllers/PostTypes.php` around lines 220 - 222, The current branch in Controllers\PostTypes.php that sets $sanitized[$key] = $setting for is_array($setting) lets any nested array pass through unsanitized; update this branch to either (a) recursively sanitize the array (walk arrays and apply the same scalar sanitization/validation used elsewhere) or (b) enforce an explicit whitelist so only known nested keys like 'conditions', 'triggers', and 'cookies' are accepted and their inner keys/values are validated; implement this change in the same method where $setting, $key and $sanitized are handled so nested arrays are never blindly assigned without validation.
163-175: Dualpopup_titleREST registration should be documented or consolidated.The
register_post_metawithshow_in_rest => true(PostTypes.php, lines 163-176) andregister_rest_field(RestAPI.php, lines 359-375) both expose the samepopup_titlemeta key in the REST API at different endpoints (/meta/popup_titleand/popup_title). While this appears intentional—onlypopup_titleuses this pattern, not other post meta—the dual registration creates API ambiguity without clear documentation explaining which endpoint clients should use.Consider either:
- Document why both registrations are necessary (block editor vs. custom namespace support)
- Consolidate to a single, well-documented REST endpoint
- Add a comment explaining the dual registration pattern
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/Controllers/PostTypes.php` around lines 163 - 175, The popup_title meta is being exposed twice via register_post_meta in PostTypes.php and register_rest_field in RestAPI.php, creating ambiguous REST endpoints; either consolidate to a single registration or add an explanatory comment and documentation. Decide the canonical exposure (prefer register_post_meta(..., show_in_rest => true) for block/editor compatibility), remove the duplicate register_rest_field registration for 'popup_title' (or set show_in_rest => false if you want the custom endpoint only), and add a brief comment in the remaining registration explaining why this meta is exposed and which REST path clients should use; update any tests or docs referencing both endpoints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@classes/Controllers/PostTypes.php`:
- Around line 171-173: The meta box auth callbacks currently call
current_user_can('edit_posts') which is inconsistent with the popup post type
permission; replace those auth_callback closures to check the popup-specific
capability by calling
current_user_can($this->container->get_permission('edit_popups')) (or otherwise
use $this->container->get_permission('edit_popups') inside the closure) so the
same permission used in the popup registration is enforced; update both
occurrences (the closure at the shown diff and the similar one around 190-192).
- Around line 220-222: The current branch in Controllers\PostTypes.php that sets
$sanitized[$key] = $setting for is_array($setting) lets any nested array pass
through unsanitized; update this branch to either (a) recursively sanitize the
array (walk arrays and apply the same scalar sanitization/validation used
elsewhere) or (b) enforce an explicit whitelist so only known nested keys like
'conditions', 'triggers', and 'cookies' are accepted and their inner keys/values
are validated; implement this change in the same method where $setting, $key and
$sanitized are handled so nested arrays are never blindly assigned without
validation.
- Around line 163-175: The popup_title meta is being exposed twice via
register_post_meta in PostTypes.php and register_rest_field in RestAPI.php,
creating ambiguous REST endpoints; either consolidate to a single registration
or add an explanatory comment and documentation. Decide the canonical exposure
(prefer register_post_meta(..., show_in_rest => true) for block/editor
compatibility), remove the duplicate register_rest_field registration for
'popup_title' (or set show_in_rest => false if you want the custom endpoint
only), and add a brief comment in the remaining registration explaining why this
meta is exposed and which REST path clients should use; update any tests or docs
referencing both endpoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5b834f5-29e5-4a48-84df-1856302d954b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
assets/js/src/admin/popup-editor/editor.scssclasses/Admin/Popups.phpclasses/Controllers/PostTypes.phptemplates/popup.php
1 participant
Summary
Changes
popup_titleREST field withupdate_callbackinRestAPI.phppopup-title-panel.tsxcomponent with title input and display toggleTest Plan
Summary by CodeRabbit
New Features
Style
Accessibility