feat(ui): add popup title panel to Block Editor document settings#1160
feat(ui): add popup title panel to Block Editor document settings#1160danieliser merged 11 commits intodevelopfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
WalkthroughAdds a popup title editing panel to the Gutenberg block editor, registers a Changes
Sequence DiagramsequenceDiagram
participant Editor as Block Editor
participant Panel as PopupTitlePanel
participant Store as Editor Data Store
participant API as REST API
participant DB as Post Meta
Editor->>Panel: Render plugin
Panel->>Store: useSelect(popup_title, postId)
Store-->>Panel: current popup_title
Panel->>Panel: set localTitle & pendingValues
Editor->>Panel: User edits title
Panel->>Panel: update localTitle & pendingValues.hasChanges = true
Editor->>Store: Save post (user triggers save)
Store->>API: Save post content (non-autosave)
API->>DB: update post content
API-->>Store: save complete
Store-->>Panel: subscription notices save finished
Panel->>API: POST /popup-maker/v2/popups/{postId} { popup_title }
API->>DB: update popup_title meta
DB-->>API: success
API-->>Panel: response
Panel->>Panel: pendingValues.hasChanges = false
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).
❌ 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
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/block-editor/src/plugins/popup-title-panel.tsx (1)
52-65: Use importedselectinstead of accessingwindow.wp.data.select.The code accesses
window.wp?.data?.selectwith verbose type casting, butselectis already imported from@wordpress/data(line 12). Using the import is cleaner and benefits from TypeScript's type inference.Proposed refactor
+import { useSelect, subscribe, select } from '@wordpress/data'; + // In setupSaveSubscription: subscribe( () => { - const editor = ( - window as typeof window & { - wp?: { - data?: { - select: ( store: string ) => EditorStore; - }; - }; - } - ).wp?.data?.select( 'core/editor' ) as EditorStore | null; - - if ( ! editor ) { - return; - } + const editor = select( 'core/editor' ) as EditorStore; const isSaving = editor.isSavingPost();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/block-editor/src/plugins/popup-title-panel.tsx` around lines 52 - 65, The subscribe callback is using a verbose window.wp.data.select lookup; replace that with the imported select from `@wordpress/data` to leverage TypeScript inference: inside the subscribe( ... ) callback, change the editor resolution to use select('core/editor') (e.g., const editor = select('core/editor') as EditorStore | null) instead of the window casting, and remove the surrounding window.wp?.data?.select cast; keep the existing null check and early return logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@classes/Controllers/PostTypes.php`:
- Around line 161-176: The auth_callback used in the register_post_meta call for
the popup meta (register_post_meta on get_type_key('popup') for 'popup_title')
checks current_user_can('edit_posts') which mismatches the popup post type's
custom capability; change the auth_callback to check the post-type specific
permission by calling current_user_can( $this->get_permission('edit_popups') )
(or equivalent method call) so meta access aligns with the popup post type
capability definitions used elsewhere.
---
Nitpick comments:
In `@packages/block-editor/src/plugins/popup-title-panel.tsx`:
- Around line 52-65: The subscribe callback is using a verbose
window.wp.data.select lookup; replace that with the imported select from
`@wordpress/data` to leverage TypeScript inference: inside the subscribe( ... )
callback, change the editor resolution to use select('core/editor') (e.g., const
editor = select('core/editor') as EditorStore | null) instead of the window
casting, and remove the surrounding window.wp?.data?.select cast; keep the
existing null check and early return logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8fa1523e-fba3-41ea-8217-2acf4ebea049
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
CHANGELOG.mdclasses/Controllers/PostTypes.phppackages/block-editor/src/plugins/popup-title-panel.tsx
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
|
|
||
| // Register popup meta for REST API (block editor support). | ||
| register_post_meta( | ||
| $this->get_type_key( 'popup' ), | ||
| 'popup_title', | ||
| [ | ||
| 'show_in_rest' => true, | ||
| 'single' => true, | ||
| 'type' => 'string', | ||
| 'sanitize_callback' => 'sanitize_text_field', | ||
| 'auth_callback' => function () { | ||
| return current_user_can( 'edit_posts' ); | ||
| }, | ||
| ] | ||
| ); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the custom capability used by popup post type
rg -nA5 "get_permission.*edit_popups" classes/Repository: PopupMaker/Popup-Maker
Length of output: 4403
🏁 Script executed:
#!/bin/bash
# Check all register_post_meta calls in PostTypes.php to see the pattern
rg -nB2 -A15 "register_post_meta" classes/Controllers/PostTypes.phpRepository: PopupMaker/Popup-Maker
Length of output: 542
🏁 Script executed:
#!/bin/bash
# Verify the file context around the auth_callback to understand closure scope
sed -n '155,180p' classes/Controllers/PostTypes.phpRepository: PopupMaker/Popup-Maker
Length of output: 697
🏁 Script executed:
#!/bin/bash
# Check if there are other auth_callbacks in the file and how they handle permissions
rg -n "auth_callback" classes/Controllers/PostTypes.php -A3Repository: PopupMaker/Popup-Maker
Length of output: 180
Use custom capability in auth_callback to match post type permissions.
The popup post type defines custom capabilities via get_permission('edit_popups') (lines 136-138), but the auth_callback for popup_title meta uses edit_posts. This creates a permission inconsistency where users with generic edit_posts capability could access the meta field when they lack edit_popups, or vice versa.
Suggested fix
register_post_meta(
$this->get_type_key( 'popup' ),
'popup_title',
[
'show_in_rest' => true,
'single' => true,
'type' => 'string',
'sanitize_callback' => 'sanitize_text_field',
'auth_callback' => function () {
- return current_user_can( 'edit_posts' );
+ return current_user_can( $this->container->get_permission( 'edit_popups' ) );
},
]
);📝 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.
| // Register popup meta for REST API (block editor support). | |
| register_post_meta( | |
| $this->get_type_key( 'popup' ), | |
| 'popup_title', | |
| [ | |
| 'show_in_rest' => true, | |
| 'single' => true, | |
| 'type' => 'string', | |
| 'sanitize_callback' => 'sanitize_text_field', | |
| 'auth_callback' => function () { | |
| return current_user_can( 'edit_posts' ); | |
| }, | |
| ] | |
| ); | |
| // Register popup meta for REST API (block editor support). | |
| register_post_meta( | |
| $this->get_type_key( 'popup' ), | |
| 'popup_title', | |
| [ | |
| 'show_in_rest' => true, | |
| 'single' => true, | |
| 'type' => 'string', | |
| 'sanitize_callback' => 'sanitize_text_field', | |
| 'auth_callback' => function () { | |
| return current_user_can( $this->container->get_permission( 'edit_popups' ) ); | |
| }, | |
| ] | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@classes/Controllers/PostTypes.php` around lines 161 - 176, The auth_callback
used in the register_post_meta call for the popup meta (register_post_meta on
get_type_key('popup') for 'popup_title') checks current_user_can('edit_posts')
which mismatches the popup post type's custom capability; change the
auth_callback to check the post-type specific permission by calling
current_user_can( $this->get_permission('edit_popups') ) (or equivalent method
call) so meta access aligns with the popup post type capability definitions used
elsewhere.
✅ Addressed in commit 7ec05ff
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
classes/Controllers/PostTypes.php (1)
162-175: Consolidatepopup_titleto a single REST write path.
popup_titleis now exposed here viaregister_post_meta(... show_in_rest => true ...), whileclasses/Controllers/RestAPI.php:343-373also exposes and writes the same field viaregister_rest_field. Keeping both paths plus the custom post-save write inpackages/block-editor/src/plugins/popup-title-panel.tsx:44-100creates split behavior and harder debugging.Pick one canonical contract (meta-backed or custom REST field) and remove the duplicate path.
Possible direction (if custom REST field remains canonical)
- // Register popup meta for REST API (block editor support). - register_post_meta( - $this->get_type_key( 'popup' ), - 'popup_title', - [ - 'show_in_rest' => true, - 'single' => true, - 'type' => 'string', - 'sanitize_callback' => 'sanitize_text_field', - 'auth_callback' => function () { - return current_user_can( $this->container->get_permission( 'edit_popups' ) ); - }, - ] - ); + // popup_title REST exposure is handled in RestAPI::register_popup_rest_fields().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/Controllers/PostTypes.php` around lines 162 - 175, Consolidate the popup_title REST handling to a single canonical path: prefer the meta-backed approach by keeping register_post_meta(...) for get_type_key('popup') and removing the duplicate custom REST field and write logic in the RestAPI class (remove the register_rest_field usage and any associated save/write handlers), then update packages/block-editor/src/plugins/popup-title-panel.tsx to read/write the post meta via the REST post meta contract instead of the custom field; ensure references to register_rest_field and any RestAPI::(method handling popup_title) are removed so only the meta-backed path remains.
🤖 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 162-175: Consolidate the popup_title REST handling to a single
canonical path: prefer the meta-backed approach by keeping
register_post_meta(...) for get_type_key('popup') and removing the duplicate
custom REST field and write logic in the RestAPI class (remove the
register_rest_field usage and any associated save/write handlers), then update
packages/block-editor/src/plugins/popup-title-panel.tsx to read/write the post
meta via the REST post meta contract instead of the custom field; ensure
references to register_rest_field and any RestAPI::(method handling popup_title)
are removed so only the meta-backed path remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f54fb289-3176-4a9a-b07a-4cda2806bb65
📒 Files selected for processing (1)
classes/Controllers/PostTypes.php
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
- 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
Move display_title logic from template into a dedicated conditional function, matching the pattern of pum_show_close_button(). Simplifies the template from 5 lines of inline logic to 2 function calls.
Remove display_title setting and toggle from both editors — popup title visibility is controlled by whether the field has a value, matching original behavior. Remove popup_settings meta registration that exposed private settings without context filtering. Keep apiFetch save pattern for popup_title — required due to custom REST namespace (popup-maker/v2) which doesn't support useEntityProp or editPost for REST field writes.
- Use pum_get_popup() object methods instead of raw post meta calls in popup_title REST field callbacks. - Show snackbar error notice on popup title save failure instead of silent console.error.
danieliser
force-pushed
the
fix/block-editor-popup-title-panel
branch
from
9ed873b to
5b2b093
Compare
2 participants
Summary
Changes
popup_titleREST field withupdate_callbackinRestAPI.phppopup-title-panel.tsxcomponent with title input and display toggleTest Plan
Summary by CodeRabbit
New Features
Style
Documentation