Necessary code for rock: product images#3055
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a wp.hooks filter check to short-circuit the admin option-reset flow, moved when non- Changes
Sequence Diagram(s)sequenceDiagram
participant AdminJS as Admin JS
participant WP_Hooks as wp.hooks
participant DOM as DOM / Browser
AdminJS->>WP_Hooks: applyFilters('frm_custom_reset_displayed_opts', false, hookArgs)
alt filter returns not false
WP_Hooks-->>AdminJS: customResult (not false)
AdminJS->>DOM: adjustConditionalLogicOptionOrders(fieldId)
AdminJS-->>DOM: return (skip default rendering)
else filter returns false
WP_Hooks-->>AdminJS: false
AdminJS->>DOM: determine container
AdminJS->>DOM: clear container (container.html(''))
AdminJS->>DOM: compute fieldInfo & render default options
end
Note over AdminJS,DOM: On product type change
AdminJS->>DOM: set data-product-type on .frm_display_format_options
AdminJS->>WP_Hooks: doAction('frm_product_type_toggled', currentVal, settings[0])
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
 |
|
Overall Grade |
Security  Reliability  Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Apr 13, 2026 2:15p.m. | Review ↗ |
| JavaScript |  |
Apr 13, 2026 2:15p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/src/admin/admin.js (1)
3779-3779: Strip the debug-only code before merge.Line 3779 logs on every toggle, and Lines 6178-6187 leave a commented debug block in
resetDisplayedOpts(). Both read like local troubleshooting rather than shipped behavior.Also applies to: 6178-6187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/admin/admin.js` at line 3779, Remove the debug-only console.log and commented troubleshooting code before merging: delete the console.log(this) call found in the toggle handler (the debug statement at/near the code that logs the component/context on each toggle) and remove the commented debug block inside the resetDisplayedOpts() function (the commented lines around the resetDisplayedOpts implementation that were used for local debugging). Ensure no other leftover debug prints or commented test blocks remain in admin.js.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/src/admin/admin.js`:
- Around line 6142-6150: The early return when a custom reset handler is
detected in resetDisplayedOpts() (the wp.hooks.applyFilters call with
'frm_custom_reset_displayed_opts' and hookArgs) prevents calling
adjustConditionalLogicOptionOrders(fieldId) and leaves conditional-logic option
lists stale; update the control flow so that when the filter returns a truthy
value indicating custom handling you still invoke
adjustConditionalLogicOptionOrders(fieldId) (or otherwise trigger the
conditional-logic sync) before returning, ensuring fields handled by the custom
renderer keep their conditional logic in sync.
---
Nitpick comments:
In `@js/src/admin/admin.js`:
- Line 3779: Remove the debug-only console.log and commented troubleshooting
code before merging: delete the console.log(this) call found in the toggle
handler (the debug statement at/near the code that logs the component/context on
each toggle) and remove the commented debug block inside the
resetDisplayedOpts() function (the commented lines around the resetDisplayedOpts
implementation that were used for local debugging). Ensure no other leftover
debug prints or commented test blocks remain in admin.js.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17901129-9a49-4447-8246-3ed08313d696
📒 Files selected for processing (2)
js/formidable_admin.jsjs/src/admin/admin.js
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
These changes look good to me.
Thanks @truongwp!
|
@truongwp One other thing I'm noticing. This update is missing the upsells that we have in Lite for radio buttons / checkboxes. 
|
truongwp
added
deploy
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/src/admin/admin.js`:
- Around line 9768-9773: The initialization dispatch uses the wrong event type:
replace the dispatched 'change' event with a delegated handler's actual trigger
so toggleImageOptions runs—either dispatch a click (e.g., dispatchEvent(new
MouseEvent('click'))) on the found .frm_toggle_image_options element, or
directly call the handler function (toggleImageOptions) with the element as
argument; update the code around the checkedDisplayFormat lookup to use one of
these two approaches so the existing delegated click listener executes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e1ceaf11-4855-4011-9ffd-8a05bbc2c298
📒 Files selected for processing (2)
js/formidable_admin.jsjs/src/admin/admin.js
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/src/admin/admin.js`:
- Around line 6146-6150: The early return in the block that checks
wp.hooks.applyFilters('frm_custom_reset_displayed_opts', ...) bypasses the
shared option-template pipeline and prevents addRadioCheckboxOpt/from
frm_admin.build_single_option_template from running; to fix, remove the early
return (or if the custom handler must short-circuit, invoke
addRadioCheckboxOpt(fieldId) and/or call frm_admin.build_single_option_template
with the appropriate args before returning) so the radio/checkbox upsell UI is
still injected while preserving adjustConditionalLogicOptionOrders(fieldId) and
the custom hook semantics.
- Line 9775: The frm_product_type_toggled action is only fired on user change;
ensure the same wp.hooks.doAction('frm_product_type_toggled', currentVal,
settings[0]) is also invoked during field initialization so consumers get the
initial state. Add a call to wp.hooks.doAction with the same arguments from the
initialization path (e.g., inside popProductFields() or the function that
applies saved settings after render) right after you set currentVal/settings so
existing saved product types trigger the hook and apply display/styling logic
immediately.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2abd4dd1-76df-4c04-a394-7bf79d9a75e8
📒 Files selected for processing (2)
js/formidable_admin.jsjs/src/admin/admin.js
| if ( false !== wp.hooks.applyFilters( 'frm_custom_reset_displayed_opts', false, hookArgs ) ) { | ||
| adjustConditionalLogicOptionOrders( fieldId ); | ||
| // Return early if there is a custom reset displayed opts handler. | ||
| return; | ||
| } |
There was a problem hiding this comment.
Don't bypass the shared option-template pipeline here.
Returning early skips the addRadioCheckboxOpt() path, so frm_admin.build_single_option_template never runs. That drops the Lite radio/checkbox upsell UI the default renderer already injects, which matches the regression being reported on this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/src/admin/admin.js` around lines 6146 - 6150, The early return in the
block that checks wp.hooks.applyFilters('frm_custom_reset_displayed_opts', ...)
bypasses the shared option-template pipeline and prevents
addRadioCheckboxOpt/from frm_admin.build_single_option_template from running; to
fix, remove the early return (or if the custom handler must short-circuit,
invoke addRadioCheckboxOpt(fieldId) and/or call
frm_admin.build_single_option_template with the appropriate args before
returning) so the radio/checkbox upsell UI is still injected while preserving
adjustConditionalLogicOptionOrders(fieldId) and the custom hook semantics.
| heading.addClass( 'frm_prod_user_def' ); | ||
| } | ||
|
|
||
| wp.hooks.doAction( 'frm_product_type_toggled', currentVal, settings[ 0 ] ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether there is any initializer besides the change handler.
rg -n -C2 '\bfrm_product_type_toggled\b|\btoggleProductType\s*\(' --type=js
rg -n -C2 "\.frmjs_prod_data_type_opt|on\( 'change', '.frmjs_prod_data_type_opt'" --type=jsRepository: Strategy11/formidable-forms
Length of output: 50384
🏁 Script executed:
# Focus on source file only, avoid minified versions
# Find where toggleProductType is bound to events
rg -n 'toggleProductType|\.on\(.*change.*product|\.frmjs_prod_data_type_opt' js/src/admin/admin.js
# Search for product field initialization or setup code
rg -n 'product_type|prod_data_type|product.*display|productField' js/src/admin/admin.js | head -40Repository: Strategy11/formidable-forms
Length of output: 555
🏁 Script executed:
# Search for initialization of product field display and any hook handlers
rg -n 'frm_product_type_toggled|popProductFields|product.*init|displayProductField' js/src/admin/admin.js
# Look for where product field settings are initially set up/displayed
rg -n -A5 'popProductFields\|product.*field.*display\|field.*options.*product' js/src/admin/admin.js | head -60Repository: Strategy11/formidable-forms
Length of output: 367
🏁 Script executed:
# Look at popProductFields function definition and surrounding context
sed -n '3280,3360p' js/src/admin/admin.js
# Look at the context around line 7691 where popProductFields is called
sed -n '7680,7710p' js/src/admin/admin.jsRepository: Strategy11/formidable-forms
Length of output: 3855
🏁 Script executed:
# Search for hook consumers/listeners that use frm_product_type_toggled
rg -n 'frm_product_type_toggled|addAction.*frm_product_type_toggled' --type=js
# Check if there's similar hook usage patterns for other field types at initialization
rg -n 'wp\.hooks\.(doAction|addAction)' js/src/admin/admin.js | grep -i 'init\|display\|toggle' | head -20Repository: Strategy11/formidable-forms
Length of output: 50384
🏁 Script executed:
# Search for any hook listeners/consumers for product-related hooks
rg -n 'addAction.*frm|applyFilters.*frm' js/src/admin/admin.js | grep -i 'product\|quantity' | head -20
# Check if there are any comments about this hook being public API
rg -n -B5 -A2 'frm_product_type_toggled' js/src/admin/admin.jsRepository: Strategy11/formidable-forms
Length of output: 336
Fire this hook for the initial state too.
The frm_product_type_toggled action only fires when the user manually changes the product type dropdown. When existing field settings are opened with a saved product type, the hook doesn't execute, so hook-based display-format styling won't apply until the user interacts with the dropdown.
Call the same hook during field initialization (e.g., in popProductFields() or similar initialization path) so that hook consumers can apply styling/setup logic for both new and existing saved product type selections.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/src/admin/admin.js` at line 9775, The frm_product_type_toggled action is
only fired on user change; ensure the same
wp.hooks.doAction('frm_product_type_toggled', currentVal, settings[0]) is also
invoked during field initialization so consumers get the initial state. Add a
call to wp.hooks.doAction with the same arguments from the initialization path
(e.g., inside popProductFields() or the function that applies saved settings
after render) right after you set currentVal/settings so existing saved product
types trigger the hook and apply display/styling logic immediately.
…ormidable-forms into rock-product-images
…e-forms into rock-product-images
When toggling the "Hide product names" checkbox, getImageLabel() always included the .frm_text_label_for_image span regardless of showLabelWithImage, diverging from the PHP-side FrmProImages::get_label() which omits the span entirely when hide_image_text is set. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Thanks @truongwp!
I made a small commit so it won't show the "Buttons" option for the Product fields placeholder.
I see the same change is in Pro, but we should have it in Lite so the upsell appears correct.
Otherwise I think we're good to merge this.
🚀
2 participants
This is necessary for https://github.com/Strategy11/formidable-pro/pull/6411
Summary by CodeRabbit
New Features
Improvements