feature: FAQ section and shortcode#309
Open
sapayth wants to merge 7 commits intoweDevsOfficial:developfrom
Open
feature: FAQ section and shortcode#309sapayth wants to merge 7 commits intoweDevsOfficial:developfrom
sapayth wants to merge 7 commits intoweDevsOfficial:developfrom
Conversation
Register wedocs_faq post type, wedocs_faq_group taxonomy, and associated meta fields. Add FAQ admin submenu page with React SPAusing tiptap rich text editor for FAQ content management.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a complete FAQ feature: new FAQ CPT/taxonomy and REST/meta, admin submenu and React-based FAQ builder, frontend shortcode/template rendering, new assets (CSS/JS + Tiptap), build entry, and a settings toggle to show FAQs on docs home. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.46)PHP Fatal error: Uncaught Error: Undefined constant "ABSPATH" in /includes/functions.php:355 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (15)
includes/Post_Types.php (1)
234-256: Consider addingauth_callbackto term meta fields for consistency.The post meta
_faq_open_by_defaulthas anauth_callbackrestricting writes to users withedit_docscapability (line 229-231), but the term meta fields (icon,order,status) don't have this protection. While the taxonomy capabilities should provide some protection, adding explicitauth_callbackfunctions would ensure consistent authorization behavior.💡 Optional: Add auth_callback to term meta
register_term_meta( 'wedocs_faq_group', 'icon', [ 'type' => 'integer', 'single' => true, 'default' => 0, 'show_in_rest' => true, 'sanitize_callback' => 'absint', + 'auth_callback' => function () { + return current_user_can( 'edit_doc_terms' ); + }, ] ); register_term_meta( 'wedocs_faq_group', 'order', [ 'type' => 'integer', 'single' => true, 'default' => 0, 'show_in_rest' => true, 'sanitize_callback' => 'absint', + 'auth_callback' => function () { + return current_user_can( 'edit_doc_terms' ); + }, ] ); register_term_meta( 'wedocs_faq_group', 'status', [ 'type' => 'boolean', 'single' => true, 'default' => true, 'show_in_rest' => true, 'sanitize_callback' => 'rest_sanitize_boolean', + 'auth_callback' => function () { + return current_user_can( 'edit_doc_terms' ); + }, ] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/Post_Types.php` around lines 234 - 256, The term meta registrations for register_term_meta on taxonomy 'wedocs_faq_group' for keys 'icon', 'order', and 'status' lack an auth_callback unlike the post meta `_faq_open_by_default`; add an auth_callback to each register_term_meta call that restricts writes to users with the same capability check used for the post meta (e.g., current_user_can('edit_docs')) so authorization is explicit and consistent across register_term_meta registrations.src/faq/components/EmptyFaq.js (1)
13-71: Minor semantic HTML issue:<h2>contains<p>elements.The heading element (
<h2>) wraps both the SVG and paragraph elements (lines 65-70). This is invalid HTML as<h2>should only contain phrasing content, not block elements like<p>.💡 Fix semantic structure
- <h2 className="mb-6"> + <div className="mb-6"> <svg xmlns="http://www.w3.org/2000/svg" ... </svg> - <p className="text-[`#3B3F4A`] font-bold text-2xl mx-auto"> + <h2 className="text-[`#3B3F4A`] font-bold text-2xl mx-auto"> { __( 'Get started by creating your first FAQ', 'wedocs' ) } - </p> + </h2> <p className="text-[`#666B79`] text-lg mx-auto mt-2"> { __( 'Create frequently asked questions to help your users find answers quickly.', 'wedocs' ) } </p> - </h2> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/faq/components/EmptyFaq.js` around lines 13 - 71, The JSX in EmptyFaq.js currently wraps block <p> elements and an <svg> inside a single <h2>, which is invalid; change the outer <h2> to a <div> (preserve className="mb-6") and create a proper heading element that contains only the title text (e.g., add a separate <h2 className="text-[`#3B3F4A`] font-bold text-2xl mx-auto"> containing { __('Get started by creating your first FAQ', 'wedocs') }). Keep the SVG and the explanatory <p className="text-[`#666B79`]..."> as sibling elements under the new wrapper so the structure is semantic and accessible.src/faq/components/AddFaqGroupModal.js (1)
78-96: wp.media availability in admin context is guaranteed—the suggested guard is optional.The
wp.mediaglobal is available in all WordPress admin pages by default, including the FAQ admin interface. SinceAddFaqGroupModalis used exclusively in the admin FAQ page, no guard is required for normal operation.However, adding a defensive check as suggested is optional refactoring that would improve robustness if the component were ever used outside the admin context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/faq/components/AddFaqGroupModal.js` around lines 78 - 96, The component assumes wp.media exists in openMediaUploader which is fine in admin but fragile if reused; add a defensive check at the top of openMediaUploader to verify wp and wp.media exist and return early (or log an error) if they don't, then proceed to create mediaUploader, attach the 'select' handler that calls setIcon with attachment.id/url/alt, and open the uploader; reference openMediaUploader, mediaUploader, and setIcon when making the change.includes/Assets.php (1)
73-82: Duplicate array keys inweDocsAdminVarslocalization (pre-existing issue).
adminUrlandhasManageCapare defined twice in this array (lines 74/76 and 75/77). In PHP, duplicate array keys cause the later value to silently overwrite the earlier one. While this doesn't break functionality here since values are identical, it indicates copy-paste error and should be cleaned up.♻️ Remove duplicate keys
wp_localize_script( 'wedocs-app-script', 'weDocsAdminVars', array( 'adminUrl' => admin_url(), 'hasManageCap' => current_user_can( 'manage_options' ), 'aiProviderConfigs' => wedocs_get_ai_provider_configs(), - 'adminUrl' => admin_url(), - 'hasManageCap' => current_user_can( 'manage_options' ), 'weDocsUrl' => admin_url( 'admin.php?page=wedocs#/' ), 'pro_active' => wedocs_is_pro_active(), 'upgradePopupContent' => wedocs_get_upgrade_popup_content(), ), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/Assets.php` around lines 73 - 82, The weDocsAdminVars localization array contains duplicate keys "adminUrl" and "hasManageCap" (in the weDocsAdminVars array emitted from Assets.php); remove the redundant entries so each key appears only once (keep one 'adminUrl' => admin_url() and one 'hasManageCap' => current_user_can('manage_options')), preserving the other keys like 'aiProviderConfigs', 'weDocsUrl', 'pro_active', and 'upgradePopupContent' and ensuring no other duplicate keys remain.assets/css/faq.css (1)
48-58: Unused transform transition on expand/collapse indicator.Line 53 applies
transition: transform 0.3s easeto the::afterpseudo-element, but the indicator usescontentchange ('+' to '−') rather than atransformto indicate state. The transition has no visual effect.Consider either removing the unused transition or switching to a rotation-based indicator for smoother visual feedback.
♻️ Option A: Remove unused transition
.wedocs-faq-item__question::after { content: '+'; font-size: 20px; font-weight: 300; color: `#6b7280`; - transition: transform 0.3s ease; }♻️ Option B: Use rotation-based indicator
.wedocs-faq-item__question::after { - content: '+'; + content: ''; + width: 12px; + height: 12px; + border-right: 2px solid `#6b7280`; + border-bottom: 2px solid `#6b7280`; + transform: rotate(45deg); - font-size: 20px; - font-weight: 300; - color: `#6b7280`; transition: transform 0.3s ease; } .wedocs-faq-item[open] .wedocs-faq-item__question::after { - content: '\2212'; + transform: rotate(-135deg); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/css/faq.css` around lines 48 - 58, The current .wedocs-faq-item__question::after uses transition: transform 0.3s ease but the visual state toggles by changing content (plus/minus), so either remove the unused transition or switch to a rotation-based indicator: Option A - delete the transition property from .wedocs-faq-item__question::after (and keep the content swap in .wedocs-faq-item[open] .wedocs-faq-item__question::after); Option B - keep a single symbol (e.g. '+') in .wedocs-faq-item__question::after, add display:inline-block and transform:rotate(0deg) plus transition:transform 0.3s ease, then in .wedocs-faq-item[open] .wedocs-faq-item__question::after replace content change with transform:rotate(45deg) or rotate(45deg)/rotate(90deg) to visually indicate open state; update the selectors .wedocs-faq-item__question::after and .wedocs-faq-item[open] .wedocs-faq-item__question::after accordingly.assets/js/faq.js (2)
5-7: Consider null-checks on queried elements.If the template markup is malformed or modified by another plugin,
summaryoranswercould benull, causing errors when accessing.styleor.addEventListener.🛡️ Add defensive null checks
document.querySelectorAll( '.wedocs-faq-item' ).forEach( function ( details ) { var summary = details.querySelector( '.wedocs-faq-item__question' ); var answer = details.querySelector( '.wedocs-faq-item__answer' ); + + if ( ! summary || ! answer ) { + return; + } // Items open by default need the inline style set so transitions work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/faq.js` around lines 5 - 7, Add defensive null-checks for queried elements inside the loop: after selecting each '.wedocs-faq-item' node and assigning variables summary and answer, verify that summary and answer are non-null before accessing their properties (like .style or calling .addEventListener). Update the block that iterates document.querySelectorAll('.wedocs-faq-item') to skip or safely handle items where summary or answer is falsy (e.g., continue the loop or wrap usages in if (summary && answer) { ... }) so malformed/modified templates don’t cause runtime errors.
17-23: Closing animation may stall iftransitionenddoesn't fire.The
transitionendhandler on lines 20-23 removes theopenattribute after the animation completes. However, if the transition doesn't fire (e.g.,prefers-reduced-motion, zero-duration transition, or element hidden during animation), the handler never executes anddetails.openremainstruedespite the visual state being closed.Consider adding a timeout fallback or listening for
prefers-reduced-motion:♻️ Add timeout fallback for reliability
if ( details.open ) { // Closing: let the transition finish before removing open. answer.style.gridTemplateRows = '0fr'; + var transitionDuration = 350; // slightly longer than CSS 0.3s + var timeoutId = setTimeout( function() { + details.open = false; + }, transitionDuration ); answer.addEventListener( 'transitionend', function handler() { + clearTimeout( timeoutId ); details.open = false; answer.removeEventListener( 'transitionend', handler ); } ); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/faq.js` around lines 17 - 23, The closing flow relying solely on the transitionend handler can stall if the transition never fires; update the block that sets answer.style.gridTemplateRows = '0fr' and the transitionend listener (the anonymous handler which sets details.open = false and removes itself) to include a reliable fallback: after attaching the transitionend listener, start a short timeout (e.g., 300ms) that will perform the same cleanup (set details.open = false and remove the listener) if transitionend doesn't fire, and also check window.matchMedia('(prefers-reduced-motion: reduce)') to skip the animation and perform immediate cleanup; ensure both paths remove the event listener and clear the timeout to avoid leaks.src/faq/components/AddFaqForm.js (1)
148-156: Add ARIA role for the toggle switch.The toggle button should have
role="switch"andaria-checkedfor better screen reader support.♿ Accessibility improvement
<button onClick={ () => setOpenByDefault( ( prev ) => ! prev ) } + role="switch" + aria-checked={ openByDefault } className={ `relative inline-flex h-5 w-9 items-center rounded-full transition-colors ${ openByDefault ? 'bg-indigo-600' : 'bg-gray-300' }` } aria-label={ __( 'Toggle open by default', 'wedocs' ) } >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/faq/components/AddFaqForm.js` around lines 148 - 156, The toggle button in AddFaqForm toggles openByDefault via setOpenByDefault but lacks ARIA switch semantics; update the button element (where setOpenByDefault and openByDefault are used) to include role="switch" and aria-checked={openByDefault} and ensure the aria-label remains descriptive (e.g., "Toggle open by default"); keep the existing onClick handler and visual classes unchanged so behavior and styling are preserved while adding the role and aria-checked attributes for screen readers.src/faq/components/FaqItem.js (2)
268-276: Add ARIA role for the toggle switch.Similar to
AddFaqForm, the toggle button should haverole="switch"andaria-checkedfor accessibility.♿ Accessibility improvement
<button onClick={ () => handleToggleOpenByDefault( ! openByDefault ) } + role="switch" + aria-checked={ openByDefault } className={ `relative inline-flex h-5 w-9 items-center rounded-full transition-colors ${ openByDefault ? 'bg-indigo-600' : 'bg-gray-300' }` } aria-label={ __( 'Toggle open by default', 'wedocs' ) } >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/faq/components/FaqItem.js` around lines 268 - 276, The toggle button in FaqItem.js lacks ARIA semantics; update the button rendered where handleToggleOpenByDefault and openByDefault are used to include role="switch" and aria-checked={openByDefault} (and ensure aria-label remains) so screen readers treat it as a switch; keep the existing onClick handler and class logic unchanged but add those ARIA attributes to the same button element.
61-91: Inconsistent answer validation compared to AddFaqForm.
handleSaveonly validates thatquestion.trim() !== ''but doesn't validate the answer field. This is inconsistent withAddFaqFormwhich requires both question and answer. Consider adding answer validation if empty answers should be rejected.🔍 Add answer validation for consistency
const handleSave = async () => { - if ( question.trim() === '' || isSaving ) { + if ( question.trim() === '' || answer.trim() === '' || isSaving ) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/faq/components/FaqItem.js` around lines 61 - 91, handleSave currently only validates question.trim() and allows empty answer values, which is inconsistent with AddFaqForm; update handleSave in FaqItem.js to also check answer.trim() along with question.trim() and isSaving before proceeding, returning early if either is empty, so the same validation logic as AddFaqForm is applied; keep the existing setIsSaving, try/catch/finally flow and ensure the UI stays in edit mode on failure and onFaqUpdated(updated) is still called on success.src/faq/components/TiptapEditor.js (1)
126-141: Consider replacingwindow.promptwith a modal dialog.Using
window.promptfor URL input works but provides a basic UX and can be blocked by browser settings. A custom modal dialog would provide better styling consistency and reliability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/faq/components/TiptapEditor.js` around lines 126 - 141, The setLink function currently uses window.prompt to collect the URL which is unreliable and offers poor UX; replace this by invoking a reusable modal dialog component that returns the entered URL (or null) asynchronously. Implement a modal (e.g., OpenUrlModal) that handles validation, accessibility and styling, call it from setLink instead of window.prompt, then keep the existing logic that checks for null (cancel) or empty string (unsetLink via editor.chain().focus().extendMarkRange('link').unsetLink().run()) and otherwise call editor.chain().focus().extendMarkRange('link').setLink({ href: url }).run(); ensure the modal returns the same semantics as prompt so existing conditional flows in setLink remain valid.includes/Admin/Menu.php (1)
60-84: Update the@sinceplaceholder before release.The
WEDOCS_SINCEplaceholder on lines 63 and 78 should be replaced with the actual version number before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/Admin/Menu.php` around lines 60 - 84, Replace the placeholder tag WEDOCS_SINCE in the docblocks for the methods faq_menu_action() and display_faq() with the actual release version string; update both occurrences in the Admin\\Menu class docblocks so the `@since` annotations show the real version number (e.g., "1.2.3") before merging.src/faq/components/FaqGroupRow.js (2)
241-269: Consider using batch deletion orPromise.allfor FAQ deletion.The sequential deletion of FAQs in a for-of loop can be slow for groups with many FAQs. Consider using
Promise.allfor parallel deletion (with appropriate rate limiting if needed).⚡ Parallel deletion for better performance
- for ( const faq of groupFaqs ) { - await apiFetch( { + await Promise.all( groupFaqs.map( ( faq ) => + apiFetch( { path: `/wp/v2/wedocs-faqs/${ faq.id }?force=true`, method: 'DELETE', - } ); - } + } ) + ) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/faq/components/FaqGroupRow.js` around lines 241 - 269, The handleDelete function currently deletes FAQs sequentially in a for-of loop which is slow for many items; refactor it to delete FAQs in parallel by mapping groupFaqs to apiFetch DELETE promises and awaiting Promise.all (or a small-batch Promise.all for rate-limiting) before deleting the group itself, ensuring you still catch errors and call setIsDeleting, setShowDeleteConfirm, and onGroupDeleted (use group.id) appropriately; reference handleDelete, groupFaqs, apiFetch, onGroupDeleted, setIsDeleting, and setShowDeleteConfirm when making the change.
357-366: Add ARIA role for the toggle switch.For consistency with accessibility standards and other toggle implementations, add
role="switch"andaria-checked.♿ Accessibility improvement
<button onClick={ handleToggle } disabled={ isToggling } + role="switch" + aria-checked={ isActive } className={ `relative inline-flex h-6 w-11 items-center rounded-full transition-colors ${ isActive ? 'bg-indigo-600' : 'bg-gray-300' } ${ isToggling ? 'opacity-50 cursor-not-allowed' : '' }` } aria-label={ isActive ? __( 'Deactivate FAQ group', 'wedocs' ) : __( 'Activate FAQ group', 'wedocs' ) } >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/faq/components/FaqGroupRow.js` around lines 357 - 366, The toggle button in the FaqGroupRow component lacks ARIA attributes for a proper switch control; update the button element used by handleToggle to include role="switch" and aria-checked={isActive} (keeping the existing aria-label and disabled handling), and ensure the props/variables isActive and isToggling are used to set those attributes so assistive tech gets the correct on/off state and disabled state.src/faq/components/FaqApp.js (1)
93-101: Consider batching the order persistence requests.The current implementation fires N separate API requests when reordering groups. For large numbers of groups, this could be slow and cause rate-limiting issues. Consider implementing a debounced batch update or a single endpoint that accepts multiple order updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/faq/components/FaqApp.js` around lines 93 - 101, The current reordered.forEach loop issues N apiFetch calls (each PATCH/POST to `/wp/v2/wedocs-faq-groups/${group.id}`) which should be replaced with a single batched request: build an array payload of { id: group.id, order: index } from reordered (preserving meta.order values), then send one apiFetch call (e.g., POST to a new batch endpoint like `/wp/v2/wedocs-faq-groups/batch` or to an existing bulk update handler) with data: { orders: [...] }; alternatively, if a server batch endpoint isn't available, debounce the reorder handler (wrap reordered processing with a short debounce) and then send fewer combined requests, ensuring you stop using reordered.forEach to fire immediate per-item requests and instead call a single function (e.g., sendGroupOrderBatch) that constructs the payload and calls apiFetch once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@includes/Shortcode.php`:
- Around line 225-237: Validate the $args['orderby'] and $args['order'] values
before placing them into $faq_query_args: check $args['orderby'] against a
whitelist of allowed orderby values (e.g.,
'none','ID','author','title','name','type','date','modified','parent','rand','menu_order','meta_value','meta_value_num','post__in')
and $args['order'] against ['ASC','DESC'] (case-insensitive), and if a value is
not allowed replace it with the safe default used by the shortcode (e.g., 'date'
and 'DESC'); perform normalization (uppercase for order) and fallback handling
where these sanitized values are then assigned to $faq_query_args['orderby'] and
$faq_query_args['order'] so WP_Query receives only validated inputs.
---
Nitpick comments:
In `@assets/css/faq.css`:
- Around line 48-58: The current .wedocs-faq-item__question::after uses
transition: transform 0.3s ease but the visual state toggles by changing content
(plus/minus), so either remove the unused transition or switch to a
rotation-based indicator: Option A - delete the transition property from
.wedocs-faq-item__question::after (and keep the content swap in
.wedocs-faq-item[open] .wedocs-faq-item__question::after); Option B - keep a
single symbol (e.g. '+') in .wedocs-faq-item__question::after, add
display:inline-block and transform:rotate(0deg) plus transition:transform 0.3s
ease, then in .wedocs-faq-item[open] .wedocs-faq-item__question::after replace
content change with transform:rotate(45deg) or rotate(45deg)/rotate(90deg) to
visually indicate open state; update the selectors
.wedocs-faq-item__question::after and .wedocs-faq-item[open]
.wedocs-faq-item__question::after accordingly.
In `@assets/js/faq.js`:
- Around line 5-7: Add defensive null-checks for queried elements inside the
loop: after selecting each '.wedocs-faq-item' node and assigning variables
summary and answer, verify that summary and answer are non-null before accessing
their properties (like .style or calling .addEventListener). Update the block
that iterates document.querySelectorAll('.wedocs-faq-item') to skip or safely
handle items where summary or answer is falsy (e.g., continue the loop or wrap
usages in if (summary && answer) { ... }) so malformed/modified templates don’t
cause runtime errors.
- Around line 17-23: The closing flow relying solely on the transitionend
handler can stall if the transition never fires; update the block that sets
answer.style.gridTemplateRows = '0fr' and the transitionend listener (the
anonymous handler which sets details.open = false and removes itself) to include
a reliable fallback: after attaching the transitionend listener, start a short
timeout (e.g., 300ms) that will perform the same cleanup (set details.open =
false and remove the listener) if transitionend doesn't fire, and also check
window.matchMedia('(prefers-reduced-motion: reduce)') to skip the animation and
perform immediate cleanup; ensure both paths remove the event listener and clear
the timeout to avoid leaks.
In `@includes/Admin/Menu.php`:
- Around line 60-84: Replace the placeholder tag WEDOCS_SINCE in the docblocks
for the methods faq_menu_action() and display_faq() with the actual release
version string; update both occurrences in the Admin\\Menu class docblocks so
the `@since` annotations show the real version number (e.g., "1.2.3") before
merging.
In `@includes/Assets.php`:
- Around line 73-82: The weDocsAdminVars localization array contains duplicate
keys "adminUrl" and "hasManageCap" (in the weDocsAdminVars array emitted from
Assets.php); remove the redundant entries so each key appears only once (keep
one 'adminUrl' => admin_url() and one 'hasManageCap' =>
current_user_can('manage_options')), preserving the other keys like
'aiProviderConfigs', 'weDocsUrl', 'pro_active', and 'upgradePopupContent' and
ensuring no other duplicate keys remain.
In `@includes/Post_Types.php`:
- Around line 234-256: The term meta registrations for register_term_meta on
taxonomy 'wedocs_faq_group' for keys 'icon', 'order', and 'status' lack an
auth_callback unlike the post meta `_faq_open_by_default`; add an auth_callback
to each register_term_meta call that restricts writes to users with the same
capability check used for the post meta (e.g., current_user_can('edit_docs')) so
authorization is explicit and consistent across register_term_meta
registrations.
In `@src/faq/components/AddFaqForm.js`:
- Around line 148-156: The toggle button in AddFaqForm toggles openByDefault via
setOpenByDefault but lacks ARIA switch semantics; update the button element
(where setOpenByDefault and openByDefault are used) to include role="switch" and
aria-checked={openByDefault} and ensure the aria-label remains descriptive
(e.g., "Toggle open by default"); keep the existing onClick handler and visual
classes unchanged so behavior and styling are preserved while adding the role
and aria-checked attributes for screen readers.
In `@src/faq/components/AddFaqGroupModal.js`:
- Around line 78-96: The component assumes wp.media exists in openMediaUploader
which is fine in admin but fragile if reused; add a defensive check at the top
of openMediaUploader to verify wp and wp.media exist and return early (or log an
error) if they don't, then proceed to create mediaUploader, attach the 'select'
handler that calls setIcon with attachment.id/url/alt, and open the uploader;
reference openMediaUploader, mediaUploader, and setIcon when making the change.
In `@src/faq/components/EmptyFaq.js`:
- Around line 13-71: The JSX in EmptyFaq.js currently wraps block <p> elements
and an <svg> inside a single <h2>, which is invalid; change the outer <h2> to a
<div> (preserve className="mb-6") and create a proper heading element that
contains only the title text (e.g., add a separate <h2 className="text-[`#3B3F4A`]
font-bold text-2xl mx-auto"> containing { __('Get started by creating your first
FAQ', 'wedocs') }). Keep the SVG and the explanatory <p
className="text-[`#666B79`]..."> as sibling elements under the new wrapper so the
structure is semantic and accessible.
In `@src/faq/components/FaqApp.js`:
- Around line 93-101: The current reordered.forEach loop issues N apiFetch calls
(each PATCH/POST to `/wp/v2/wedocs-faq-groups/${group.id}`) which should be
replaced with a single batched request: build an array payload of { id:
group.id, order: index } from reordered (preserving meta.order values), then
send one apiFetch call (e.g., POST to a new batch endpoint like
`/wp/v2/wedocs-faq-groups/batch` or to an existing bulk update handler) with
data: { orders: [...] }; alternatively, if a server batch endpoint isn't
available, debounce the reorder handler (wrap reordered processing with a short
debounce) and then send fewer combined requests, ensuring you stop using
reordered.forEach to fire immediate per-item requests and instead call a single
function (e.g., sendGroupOrderBatch) that constructs the payload and calls
apiFetch once.
In `@src/faq/components/FaqGroupRow.js`:
- Around line 241-269: The handleDelete function currently deletes FAQs
sequentially in a for-of loop which is slow for many items; refactor it to
delete FAQs in parallel by mapping groupFaqs to apiFetch DELETE promises and
awaiting Promise.all (or a small-batch Promise.all for rate-limiting) before
deleting the group itself, ensuring you still catch errors and call
setIsDeleting, setShowDeleteConfirm, and onGroupDeleted (use group.id)
appropriately; reference handleDelete, groupFaqs, apiFetch, onGroupDeleted,
setIsDeleting, and setShowDeleteConfirm when making the change.
- Around line 357-366: The toggle button in the FaqGroupRow component lacks ARIA
attributes for a proper switch control; update the button element used by
handleToggle to include role="switch" and aria-checked={isActive} (keeping the
existing aria-label and disabled handling), and ensure the props/variables
isActive and isToggling are used to set those attributes so assistive tech gets
the correct on/off state and disabled state.
In `@src/faq/components/FaqItem.js`:
- Around line 268-276: The toggle button in FaqItem.js lacks ARIA semantics;
update the button rendered where handleToggleOpenByDefault and openByDefault are
used to include role="switch" and aria-checked={openByDefault} (and ensure
aria-label remains) so screen readers treat it as a switch; keep the existing
onClick handler and class logic unchanged but add those ARIA attributes to the
same button element.
- Around line 61-91: handleSave currently only validates question.trim() and
allows empty answer values, which is inconsistent with AddFaqForm; update
handleSave in FaqItem.js to also check answer.trim() along with question.trim()
and isSaving before proceeding, returning early if either is empty, so the same
validation logic as AddFaqForm is applied; keep the existing setIsSaving,
try/catch/finally flow and ensure the UI stays in edit mode on failure and
onFaqUpdated(updated) is still called on success.
In `@src/faq/components/TiptapEditor.js`:
- Around line 126-141: The setLink function currently uses window.prompt to
collect the URL which is unreliable and offers poor UX; replace this by invoking
a reusable modal dialog component that returns the entered URL (or null)
asynchronously. Implement a modal (e.g., OpenUrlModal) that handles validation,
accessibility and styling, call it from setLink instead of window.prompt, then
keep the existing logic that checks for null (cancel) or empty string (unsetLink
via editor.chain().focus().extendMarkRange('link').unsetLink().run()) and
otherwise call editor.chain().focus().extendMarkRange('link').setLink({ href:
url }).run(); ensure the modal returns the same semantics as prompt so existing
conditional flows in setLink remain valid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28ab20c7-9a76-4d58-b3df-8d550549cbb6
📒 Files selected for processing (24)
assets/css/faq.cssassets/js/faq.jsincludes/Admin/Menu.phpincludes/Assets.phpincludes/Frontend.phpincludes/Post_Types.phpincludes/Shortcode.phppackage.jsonreadme.txtsrc/components/Settings/GeneralSettings.jssrc/faq/assets/faq.csssrc/faq/components/AddFaqForm.jssrc/faq/components/AddFaqGroupModal.jssrc/faq/components/EmptyFaq.jssrc/faq/components/FaqApp.jssrc/faq/components/FaqConfirmDialog.jssrc/faq/components/FaqGroupRow.jssrc/faq/components/FaqItem.jssrc/faq/components/TiptapEditor.jssrc/faq/index.jstemplates/admin/faq.phptemplates/shortcode-faq.phptemplates/shortcode.phpwebpack.config.js
Comment on lines
+225
to
+237
| $faq_query_args = [ | ||
| 'post_type' => 'wedocs_faq', | ||
| 'posts_per_page' => (int) $args['limit'], | ||
| 'orderby' => $args['orderby'], | ||
| 'order' => $args['order'], | ||
| 'tax_query' => [ // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_tax_query | ||
| [ | ||
| 'taxonomy' => 'wedocs_faq_group', | ||
| 'field' => 'term_id', | ||
| 'terms' => $group->term_id, | ||
| ], | ||
| ], | ||
| ]; |
Contributor
There was a problem hiding this comment.
Validate orderby and order attributes against allowed values.
The orderby and order shortcode attributes are passed directly to WP_Query without validation. While WordPress sanitizes these internally, explicitly validating against allowed values prevents unexpected behavior and potential information disclosure.
🛡️ Proposed fix
+ $allowed_orderby = [ 'menu_order', 'title', 'date', 'ID' ];
+ $allowed_order = [ 'ASC', 'DESC' ];
+
$faq_query_args = [
'post_type' => 'wedocs_faq',
'posts_per_page' => (int) $args['limit'],
- 'orderby' => $args['orderby'],
- 'order' => $args['order'],
+ 'orderby' => in_array( $args['orderby'], $allowed_orderby, true ) ? $args['orderby'] : 'menu_order',
+ 'order' => in_array( strtoupper( $args['order'] ), $allowed_order, true ) ? strtoupper( $args['order'] ) : 'ASC',
'tax_query' => [ // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_tax_query📝 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.
Suggested change
| $faq_query_args = [ | |
| 'post_type' => 'wedocs_faq', | |
| 'posts_per_page' => (int) $args['limit'], | |
| 'orderby' => $args['orderby'], | |
| 'order' => $args['order'], | |
| 'tax_query' => [ // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_tax_query | |
| [ | |
| 'taxonomy' => 'wedocs_faq_group', | |
| 'field' => 'term_id', | |
| 'terms' => $group->term_id, | |
| ], | |
| ], | |
| ]; | |
| $allowed_orderby = [ 'menu_order', 'title', 'date', 'ID' ]; | |
| $allowed_order = [ 'ASC', 'DESC' ]; | |
| $faq_query_args = [ | |
| 'post_type' => 'wedocs_faq', | |
| 'posts_per_page' => (int) $args['limit'], | |
| 'orderby' => in_array( $args['orderby'], $allowed_orderby, true ) ? $args['orderby'] : 'menu_order', | |
| 'order' => in_array( strtoupper( $args['order'] ), $allowed_order, true ) ? strtoupper( $args['order'] ) : 'ASC', | |
| 'tax_query' => [ // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_tax_query | |
| [ | |
| 'taxonomy' => 'wedocs_faq_group', | |
| 'field' => 'term_id', | |
| 'terms' => $group->term_id, | |
| ], | |
| ], | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/Shortcode.php` around lines 225 - 237, Validate the $args['orderby']
and $args['order'] values before placing them into $faq_query_args: check
$args['orderby'] against a whitelist of allowed orderby values (e.g.,
'none','ID','author','title','name','type','date','modified','parent','rand','menu_order','meta_value','meta_value_num','post__in')
and $args['order'] against ['ASC','DESC'] (case-insensitive), and if a value is
not allowed replace it with the safe default used by the shortcode (e.g., 'date'
and 'DESC'); perform normalization (uppercase for order) and fallback handling
where these sanitized values are then assigned to $faq_query_args['orderby'] and
$faq_query_args['order'] so WP_Query receives only validated inputs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #314, closes #315, closes #221
Summary
Adds a full FAQ module to weDocs, allowing users to create and manage FAQs grouped by category directly from the WordPress admin.
What's included
wedocs_faqpost type andwedocs_faq_grouptaxonomy for structured FAQ storagesrc/faq/) with a rich text editor (Tiptap) for FAQ content@dnd-kit[wedocs_faq]shortcode for frontend display with accordion-style expand/collapsegroup,limit,orderby, andorderattributeswedocs_faq_shortcode_query_args,wedocs_get_faq_listing_template_dir, andwedocs_get_faq_listing_template_argshooksassets/css/faq.css,assets/js/faq.js) for accordion behavior and stylingTest plan
[wedocs_faq]shortcode on a page and verify FAQ accordion renders correctlygroup="slug",limit="5",orderby="date",order="DESC"Summary by CodeRabbit
New Features
Chores