feat: Admin Settings Restructure #2793
Conversation
…ommission, CombineInput, Checkbox, Currency, MultiCheck, Number, Password, Radio, RadioBox, Select, Switcher, Tel, Text, and FieldGroup
WalkthroughThis change introduces a comprehensive restructuring of the admin settings system. It adds new classes for settings elements, pages, factories, mappers, and transformers, implements a REST API controller for admin settings, and updates dependency management to register and expose these new services. The control flow for settings retrieval, transformation, and persistence is refactored. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUI
participant RESTController
participant SettingsManager
participant SettingsPage
participant Transformer
AdminUI->>RESTController: GET /settings
RESTController->>SettingsManager: get_pages_data()
SettingsManager->>SettingsPage: settings()
SettingsPage-->>SettingsManager: settings array
SettingsManager->>Transformer: transform(settings)
Transformer-->>SettingsManager: UI elements
SettingsManager-->>RESTController: data
RESTController-->>AdminUI: JSON response
AdminUI->>RESTController: POST /settings (updated data)
RESTController->>SettingsManager: save(parsed data)
SettingsManager->>SettingsPage: save(data)
SettingsPage-->>SettingsManager: (save complete)
SettingsManager-->>RESTController: updated data
RESTController-->>AdminUI: JSON response
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested labels
Suggested reviewers
Poem
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. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (31)
includes/Admin/Settings/Elements/Fields/RadioBox.php (1)
5-9: Fix class documentation inconsistency.The class docblock refers to "RadioButton Field" but the class name is
RadioBox. Update the documentation to match the class name.-/** - * RadioButton Field. - * - * Custom field that provides radio options as button. - */ +/** + * RadioBox Field. + * + * Custom field that provides radio options as buttons. + */includes/Admin/Settings/Elements/Fields/Tel.php (2)
5-7: Fix incorrect class documentation.The class docblock incorrectly refers to "Test Field" instead of "Tel Field" or "Telephone Field".
-/** - * Test Field. - */ +/** + * Telephone Field. + * + * Input field for telephone numbers. + */
24-26: Consider enhancing telephone number validation.The current validation only checks if data is set, which is quite basic for a telephone field. Consider implementing more specific validation for telephone number formats.
For example, you could add basic format validation:
public function data_validation( $data ): bool { - return isset( $data ); + if ( ! isset( $data ) || empty( $data ) ) { + return false; + } + + // Basic phone number validation (digits, spaces, hyphens, parentheses, plus) + return preg_match( '/^[\d\s\-\(\)\+]+$/', $data ); }includes/Admin/Settings/Elements/Fields/Password.php (2)
5-7: Fix incorrect class comment.The comment says "Test Field" but this is actually a Password field class.
-/** - * Test Field. - */ +/** + * Password Field. + */
24-26: Consider enhancing password field validation.The current validation only checks if data is set, which may be too permissive for password fields. Consider adding password-specific validation such as minimum length requirements, complexity rules, or other security constraints based on your application's password policy.
public function data_validation( $data ): bool { - return isset( $data ); + // Add password-specific validation if needed + // e.g., minimum length, complexity requirements + return isset( $data ) && is_string( $data ) && strlen( trim( $data ) ) > 0; }includes/Admin/Settings/Elements/Section.php (1)
1-53: LGTM: Consistent implementation with minor documentation improvements needed.The
Sectionclass follows the established pattern for settings elements. The implementation is consistent with other container-type elements likeTab,SubPage, andSubSection.Minor suggestions:
- Consider adding more descriptive class documentation explaining the purpose of a "section" element
- The
sanitize_elementmethod return type annotation showsarray|stringbut only returns the result ofwp_unslash()- consider clarifying the expected return type/** - * Section Class. + * Section settings element class for grouping related settings. + * + * Represents a container element that groups related settings fields, + * typically used to organize settings within a tab or page. */ class Section extends SettingsElement {includes/Admin/Settings/Elements/Tab.php (1)
1-52: LGTM: Consistent implementation with documentation enhancement opportunity.The
Tabclass properly follows the established pattern for settings elements and is consistent with other container-type elements.Consider enhancing the class documentation to better explain the role of tab elements in the settings hierarchy:
/** - * Tab Class. + * Tab settings element class for organizing settings into tabbed interfaces. + * + * Represents a tab container that groups related settings, typically used + * to organize settings into separate tabs within a settings page. */ class Tab extends SettingsElement {includes/Admin/Dashboard/Pages/Settings.php (1)
38-47: Consider using the position parameter.The
menu()method accepts a$positionparameter but hardcodes the position to 5. Consider using the parameter or remove it if not needed.public function menu( string $capability, string $position ): array { return [ 'page_title' => __( 'Admin Settings', 'dokan-lite' ), 'menu_title' => __( 'Settings', 'dokan-lite' ), 'route' => $this->get_id(), 'capability' => $capability, - 'position' => 5, + 'position' => (int) $position, 'hidden' => false, ]; }includes/Admin/Settings/Pages/PageInterface.php (1)
10-59: Update @SInCE placeholder tags before release.The interface design is well-structured with clear method contracts. However, all
@sincetags currently use the placeholderDOKAN_SINCEwhich should be replaced with the actual version number before release.Replace all instances of
DOKAN_SINCEwith the actual version:- * @since DOKAN_SINCE + * @since 3.x.xincludes/Admin/Settings/Elements/Fields/Switcher.php (1)
63-70: Add missing PHPDoc comment for consistency.The
set_disable_statemethod is missing its PHPDoc comment block, which breaks consistency with other methods in the class.Add the missing PHPDoc comment:
+ /** + * Set disable state. + * + * @param string $label Disable state label. + * @param string $value Disable state value. + * + * @return Switcher + */ public function set_disable_state( string $label, string $value ): Switcher {includes/Admin/Settings/Elements/SubPage.php (2)
7-9: Fix incorrect class name in docblock.The docblock comment refers to "Page Class" but this is the
SubPageclass.- * Page Class. + * SubPage Class.
35-35: Fix inconsistent return type documentation.The return type annotation shows
array|stringbut the actual return type and usage pattern (consistent with other elements) suggests it should return the same type as input.- * @return array|string + * @return mixedincludes/Admin/Settings/Elements/Fields/Select.php (1)
8-8: Consider inheritance hierarchy - Select extending Checkbox may be confusing.The inheritance choice is unusual since
Selectfields typically handle single values (non-array) whileCheckboxfields handle multiple values (array data). This creates a semantic inconsistency in the class hierarchy.Consider making
SelectextendTextorFielddirectly, or create a common base class for option-based fields:-class Select extends Checkbox { +class Select extends Text {includes/Admin/Settings/Settings.php (3)
80-82: Remove unused variable flagged by static analysis.The
$indexvariable in the foreach loop is unused and should be removed.-foreach ( $pages as $index => $page ) { +foreach ( $pages as $page ) { $mapped_pages[] = $page->populate(); }
30-31: Document the InvalidArgumentException in method signature.The method throws
\InvalidArgumentExceptionbut this isn't documented in the@throwsannotation, which is inconsistent with the docblock style used elsewhere./** * Get all pages. * * @since DOKAN_SINCE * * @return array< AbstractPage > * + * @throws \InvalidArgumentException If any page is not an instance of AbstractPage. - * @throws \InvalidArgumentException If the step is not an instance of AbstractPage. */
46-48: Improve filter validation robustness.The current validation simply returns the original pages if the filtered result isn't an array. Consider logging this scenario or providing more explicit handling.
if ( ! is_array( $pages ) ) { + // Log unexpected filter result for debugging + error_log( 'dokan_admin_settings_pages filter returned non-array value' ); return $this->pages; }includes/Admin/Settings/Elements/Fields/Commission/CategoryBasedCommission.php (1)
75-75: Consider dependency injection for Categories controller.Creating a new
Categoriesinstance in thepopulate()method on every call could impact performance and makes testing more difficult.Consider dependency injection or lazy loading:
+private $categories_controller; +private function get_categories_controller(): Categories { + if ( ! $this->categories_controller ) { + $this->categories_controller = new Categories(); + } + return $this->categories_controller; +} public function populate(): array { - $categories_controller = new Categories(); + $categories_controller = $this->get_categories_controller(); $data = parent::populate();includes/Admin/Settings/Pages/AbstractPage.php (2)
25-37: Replace placeholder version tags with actual version.The
@since DOKAN_SINCEplaceholder should be replaced with the actual version number.- * @since DOKAN_SINCE + * @since 3.x.x
88-88: Use a constant for the filter name to improve maintainability.Hard-coded filter names make refactoring difficult and prone to typos.
Consider defining this as a class constant:
private const SETTINGS_PAGES_FILTER = 'dokan_admin_settings_pages';Then use:
- add_filter( 'dokan_admin_settings_pages', [ $this, 'enlist' ] ); + add_filter( self::SETTINGS_PAGES_FILTER, [ $this, 'enlist' ] );includes/Admin/Settings/Elements/Fields/Commission/CombineInput.php (3)
24-31: Consider using appropriate numeric types for fee and percentage values.Using
stringfor numeric values like fees and percentages may lead to type confusion and calculation errors.- protected $additional_fee = ''; - protected $admin_percentage = ''; + protected float $additional_fee = 0.0; + protected float $admin_percentage = 0.0;Update corresponding getter/setter methods accordingly.
119-119: Fix inconsistent indentation.Line 119 uses tab indentation while the rest of the file uses spaces.
- return esc_html( $data ); + return esc_html( $data );
76-76: Fix parameter naming inconsistency.Parameter
$percentageshould match the property name$admin_percentagefor clarity.- * @param string $percentage Percentage value. + * @param string $admin_percentage Percentage value.- public function set_admin_percentage( string $percentage ): CombineInput { + public function set_admin_percentage( string $admin_percentage ): CombineInput {- $this->admin_percentage = $percentage; + $this->admin_percentage = $admin_percentage;includes/Admin/Settings/Elements/ElementFactory.php (1)
69-75: Simplify the return type annotation and consider implementation efficiency.The return type annotation is very long and the implementation creates a Field object just to call
get_input().- * @return Text|Number|Checkbox|Radio|Select|Tel|Password|RadioBox|Switcher|MultiCheck|Currency + * @return SettingsElement */ public static function field( string $id, string $type = 'text' ): SettingsElement { - $field = new Field( $id, $type ); - - return $field->get_input(); + return ( new Field( $id, $type ) )->get_input(); }includes/Admin/Settings/Elements/Fields/Text.php (1)
8-10: Fix documentation typo.Class documentation has a typo: "Test Field" should be "Text Field".
/** - * Test Field. + * Text Field. */includes/Admin/Settings/Elements/Fields/Number.php (3)
24-29: Fix PHPDoc format for consistency.The PHPDoc is missing the property name in the annotation.
Apply this diff to fix the PHPDoc:
/** * Maximum. * - * @var float Maximum. + * @var float $maximum Maximum. */ protected $maximum;
38-45: Fix PHPDoc return type to match nullable return./** * Get minimum value. * - * @return float + * @return float|null */ public function get_minimum(): ?float {
82-89: Fix PHPDoc return type to match nullable return./** * Get step value. * - * @return float + * @return float|null */ public function get_step(): ?float {includes/REST/AdminSettingsController.php (1)
20-25: Replace version placeholder with actual version./** * Register all routes. * - * @since DOKAN_SINCE + * @since 3.8.0 * * @return void */includes/Admin/Settings/ElementTransformer.php (3)
3-6: Remove redundant namespace in use statement.
TransformerInterfaceis in the same namespace, so the full namespace is not needed.namespace WeDevs\Dokan\Admin\Settings; -use WeDevs\Dokan\Admin\Settings\TransformerInterface; +use TransformerInterface; use WeDevs\Dokan\Admin\Settings\Elements\ElementFactory;Or simply remove the use statement and use
TransformerInterfacedirectly since it's in the same namespace.
8-18: Remove unused property.The
$settingsproperty is declared but never used in the class.class ElementTransformer implements TransformerInterface { /** * @var array<string, array> $sections */ protected array $sections = []; /** * @var array<string, array> $fields */ protected array $fields = []; - protected $settings;
106-116: Consider logging when fallback types are used.When unsupported field types fall back to 'text', it would be helpful to log this for debugging.
default: // Map some field types to their correct names $type_mapping = [ 'switcher' => 'switch', 'wpeditor' => 'text', // Fallback for unsupported types 'file' => 'text', // Fallback for unsupported types ]; $mapped_type = $type_mapping[ $type ] ?? $type; + + // Log when using fallback types + if ( in_array( $type, [ 'wpeditor', 'file' ], true ) ) { + // Consider adding logging here + // do_action( 'dokan_admin_settings_unsupported_field_type', $type, $field_id ); + } + $element = ElementFactory::field( $field_id, $mapped_type ); break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
includes/Abstracts/Settings.php(1 hunks)includes/Admin/Dashboard/Pages/Settings.php(1 hunks)includes/Admin/Settings/ElementTransformer.php(1 hunks)includes/Admin/Settings/Elements/ElementFactory.php(1 hunks)includes/Admin/Settings/Elements/Field.php(1 hunks)includes/Admin/Settings/Elements/FieldGroup.php(1 hunks)includes/Admin/Settings/Elements/Fields/Checkbox.php(1 hunks)includes/Admin/Settings/Elements/Fields/Commission/CategoryBasedCommission.php(1 hunks)includes/Admin/Settings/Elements/Fields/Commission/CombineInput.php(1 hunks)includes/Admin/Settings/Elements/Fields/Currency.php(1 hunks)includes/Admin/Settings/Elements/Fields/MultiCheck.php(1 hunks)includes/Admin/Settings/Elements/Fields/Number.php(1 hunks)includes/Admin/Settings/Elements/Fields/Password.php(1 hunks)includes/Admin/Settings/Elements/Fields/Radio.php(1 hunks)includes/Admin/Settings/Elements/Fields/RadioBox.php(1 hunks)includes/Admin/Settings/Elements/Fields/Select.php(1 hunks)includes/Admin/Settings/Elements/Fields/Switcher.php(1 hunks)includes/Admin/Settings/Elements/Fields/Tel.php(1 hunks)includes/Admin/Settings/Elements/Fields/Text.php(1 hunks)includes/Admin/Settings/Elements/Section.php(1 hunks)includes/Admin/Settings/Elements/SubPage.php(1 hunks)includes/Admin/Settings/Elements/SubSection.php(1 hunks)includes/Admin/Settings/Elements/Tab.php(1 hunks)includes/Admin/Settings/MapperInterface.php(1 hunks)includes/Admin/Settings/Pages/AbstractPage.php(1 hunks)includes/Admin/Settings/Pages/PageInterface.php(1 hunks)includes/Admin/Settings/Settings.php(1 hunks)includes/Admin/Settings/TransformerInterface.php(1 hunks)includes/DependencyManagement/Providers/AdminDashboardServiceProvider.php(2 hunks)includes/DependencyManagement/Providers/AdminSettingsServiceProvider.php(1 hunks)includes/DependencyManagement/Providers/ServiceProvider.php(1 hunks)includes/REST/AdminSettingsController.php(1 hunks)includes/REST/AdminSetupGuideController.php(3 hunks)includes/REST/Manager.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (17)
includes/Admin/Settings/Elements/Fields/RadioBox.php (1)
includes/Admin/Settings/Elements/Fields/Radio.php (1)
Radio(8-51)
includes/Admin/Settings/TransformerInterface.php (1)
includes/Admin/Settings/ElementTransformer.php (3)
get_target(22-24)transform(29-59)set_settings(64-78)
includes/Admin/Settings/Elements/FieldGroup.php (5)
includes/Admin/Settings/Elements/Field.php (3)
data_validation(107-109)sanitize_element(148-150)escape_element(159-161)includes/Admin/Settings/Elements/Section.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(49-51)includes/Admin/Settings/Elements/Tab.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(48-50)includes/Admin/Settings/Elements/SubPage.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(49-51)includes/Admin/Settings/Elements/SubSection.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(49-51)
includes/DependencyManagement/Providers/ServiceProvider.php (5)
lib/packages/League/Container/ContainerAwareInterface.php (1)
getContainer(9-9)lib/packages/League/Container/ContainerAwareTrait.php (1)
getContainer(32-39)lib/packages/League/Container/Container.php (1)
addServiceProvider(103-107)lib/packages/League/Container/DefinitionContainerInterface.php (1)
addServiceProvider(15-15)includes/DependencyManagement/Providers/AdminSettingsServiceProvider.php (1)
AdminSettingsServiceProvider(8-30)
includes/Admin/Settings/Elements/Fields/Switcher.php (3)
includes/Admin/Settings/Elements/Fields/Radio.php (2)
Radio(8-51)populate(44-50)includes/Admin/Settings/Elements/Field.php (1)
populate(92-98)includes/Admin/Settings/Elements/Fields/Checkbox.php (1)
populate(91-98)
includes/Admin/Settings/Elements/Fields/Select.php (15)
includes/Admin/Settings/Elements/Fields/Checkbox.php (5)
Checkbox(10-99)data_validation(71-73)escape_element(82-84)populate(91-98)get_options(31-33)includes/Admin/Settings/Elements/Fields/MultiCheck.php (4)
data_validation(111-113)populate(120-127)get_default(87-89)get_options(48-50)includes/Admin/Settings/Elements/FieldGroup.php (2)
data_validation(26-28)escape_element(48-50)includes/Admin/Settings/Elements/Field.php (3)
data_validation(107-109)escape_element(159-161)populate(92-98)includes/Admin/Settings/Elements/Fields/Commission/CategoryBasedCommission.php (3)
data_validation(65-67)escape_element(91-93)populate(74-82)includes/Admin/Settings/Elements/Fields/Commission/CombineInput.php (3)
data_validation(93-95)escape_element(118-120)populate(102-109)includes/Admin/Settings/Elements/Fields/Password.php (2)
data_validation(24-26)populate(33-39)includes/Admin/Settings/Elements/Fields/Number.php (3)
data_validation(111-116)escape_element(137-139)populate(146-155)includes/Admin/Settings/Elements/Fields/Radio.php (3)
data_validation(24-26)escape_element(35-37)populate(44-50)includes/Admin/Settings/Elements/Fields/Tel.php (2)
data_validation(24-26)populate(33-39)includes/Admin/Settings/Elements/Section.php (2)
data_validation(26-28)escape_element(49-51)includes/Admin/Settings/Elements/Fields/Text.php (4)
data_validation(174-176)escape_element(212-214)populate(183-192)get_default(62-64)includes/Admin/Settings/Elements/Tab.php (2)
data_validation(26-28)escape_element(48-50)includes/Admin/Settings/Elements/SubPage.php (2)
data_validation(26-28)escape_element(49-51)includes/Admin/Settings/Elements/SubSection.php (2)
data_validation(26-28)escape_element(49-51)
includes/Admin/Settings/Elements/Section.php (5)
includes/Admin/Settings/Elements/FieldGroup.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(48-50)includes/Admin/Settings/Elements/Field.php (3)
data_validation(107-109)sanitize_element(148-150)escape_element(159-161)includes/Admin/Settings/Elements/Tab.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(48-50)includes/Admin/Settings/Elements/SubPage.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(49-51)includes/Admin/Settings/Elements/SubSection.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(49-51)
includes/Admin/Settings/Elements/SubPage.php (5)
includes/Admin/Settings/Elements/FieldGroup.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(48-50)includes/Admin/Settings/Elements/Field.php (3)
data_validation(107-109)sanitize_element(148-150)escape_element(159-161)includes/Admin/Settings/Elements/Section.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(49-51)includes/Admin/Settings/Elements/Tab.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(48-50)includes/Admin/Settings/Elements/SubSection.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(49-51)
includes/Admin/Settings/Elements/Fields/Radio.php (5)
includes/Admin/Settings/Elements/Fields/Checkbox.php (5)
Checkbox(10-99)data_validation(71-73)escape_element(82-84)populate(91-98)get_options(31-33)includes/Admin/Settings/Elements/Field.php (3)
data_validation(107-109)escape_element(159-161)populate(92-98)includes/Admin/Settings/Elements/Fields/Password.php (2)
data_validation(24-26)populate(33-39)includes/Admin/Settings/Elements/Fields/Select.php (3)
data_validation(24-26)escape_element(35-37)populate(44-49)includes/Admin/Settings/Elements/Fields/Tel.php (2)
data_validation(24-26)populate(33-39)
includes/Admin/Settings/Elements/Tab.php (5)
includes/Admin/Settings/Elements/FieldGroup.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(48-50)includes/Admin/Settings/Elements/Field.php (3)
data_validation(107-109)sanitize_element(148-150)escape_element(159-161)includes/Admin/Settings/Elements/Section.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(49-51)includes/Admin/Settings/Elements/SubPage.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(49-51)includes/Admin/Settings/Elements/SubSection.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(49-51)
includes/Admin/Settings/Elements/Fields/MultiCheck.php (6)
includes/Admin/Settings/Elements/Field.php (4)
Field(23-162)__construct(73-76)data_validation(107-109)populate(92-98)includes/Admin/Settings/Elements/Fields/Commission/CombineInput.php (3)
__construct(38-40)data_validation(93-95)populate(102-109)includes/Admin/Settings/Elements/Fields/Checkbox.php (5)
get_options(31-33)set_options(42-46)add_option(56-62)data_validation(71-73)populate(91-98)includes/Admin/Settings/Elements/Fields/Number.php (2)
data_validation(111-116)populate(146-155)includes/Admin/Settings/Elements/Fields/Radio.php (2)
data_validation(24-26)populate(44-50)includes/Admin/Settings/Elements/Fields/Select.php (2)
data_validation(24-26)populate(44-49)
includes/REST/AdminSetupGuideController.php (2)
includes/Admin/OnboardingSetup/Steps/AbstractStep.php (1)
AbstractStep(13-257)includes/Abstracts/Settings.php (1)
populate_children_only(53-56)
includes/Admin/Settings/Elements/Fields/Commission/CategoryBasedCommission.php (2)
includes/Admin/Settings/Elements/Field.php (5)
Field(23-162)__construct(73-76)data_validation(107-109)populate(92-98)escape_element(159-161)includes/ProductCategory/Categories.php (1)
Categories(6-208)
includes/Admin/Settings/Elements/Field.php (16)
includes/Admin/Settings/Elements/Fields/Checkbox.php (4)
Checkbox(10-99)populate(91-98)escape_element(82-84)data_validation(71-73)includes/Admin/Settings/Elements/Fields/Commission/CategoryBasedCommission.php (5)
CategoryBasedCommission(11-94)__construct(32-34)populate(74-82)escape_element(91-93)data_validation(65-67)includes/Admin/Settings/Elements/Fields/Commission/CombineInput.php (5)
CombineInput(10-121)__construct(38-40)populate(102-109)escape_element(118-120)data_validation(93-95)includes/Admin/Settings/Elements/Fields/Currency.php (2)
Currency(5-58)populate(26-35)includes/Admin/Settings/Elements/Fields/MultiCheck.php (4)
MultiCheck(11-128)__construct(39-41)populate(120-127)data_validation(111-113)includes/Admin/Settings/Elements/Fields/Number.php (5)
Number(8-156)populate(146-155)escape_element(137-139)data_validation(111-116)sanitize_element(126-128)includes/Admin/Settings/Elements/Fields/Radio.php (4)
Radio(8-51)populate(44-50)escape_element(35-37)data_validation(24-26)includes/Admin/Settings/Elements/Fields/RadioBox.php (1)
RadioBox(10-18)includes/Admin/Settings/Elements/Fields/Select.php (4)
Select(8-50)populate(44-49)escape_element(35-37)data_validation(24-26)includes/Admin/Settings/Elements/Fields/Switcher.php (2)
Switcher(12-95)populate(87-94)includes/Admin/Settings/Elements/Fields/Text.php (6)
Text(11-215)__construct(53-55)populate(183-192)escape_element(212-214)data_validation(174-176)sanitize_element(201-203)includes/Admin/Settings/Elements/FieldGroup.php (3)
escape_element(48-50)data_validation(26-28)sanitize_element(37-39)includes/Admin/Settings/Elements/Section.php (3)
escape_element(49-51)data_validation(26-28)sanitize_element(37-39)includes/Admin/Settings/Elements/Tab.php (3)
escape_element(48-50)data_validation(26-28)sanitize_element(37-39)includes/Admin/Settings/Elements/SubPage.php (3)
escape_element(49-51)data_validation(26-28)sanitize_element(37-39)includes/Admin/Settings/Elements/SubSection.php (3)
escape_element(49-51)data_validation(26-28)sanitize_element(37-39)
includes/Admin/Settings/Elements/Fields/Currency.php (1)
includes/Admin/Settings/Elements/Fields/Text.php (6)
Text(11-215)populate(183-192)get_default(62-64)get_placeholder(84-86)is_readonly(106-108)is_disabled(128-130)
includes/Admin/Settings/Elements/SubSection.php (5)
includes/Admin/Settings/Elements/FieldGroup.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(48-50)includes/Admin/Settings/Elements/Field.php (3)
data_validation(107-109)sanitize_element(148-150)escape_element(159-161)includes/Admin/Settings/Elements/Section.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(49-51)includes/Admin/Settings/Elements/Tab.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(48-50)includes/Admin/Settings/Elements/SubPage.php (3)
data_validation(26-28)sanitize_element(37-39)escape_element(49-51)
includes/Admin/Settings/Elements/Fields/Number.php (1)
includes/Admin/Settings/Elements/Fields/Text.php (7)
Text(11-215)data_validation(174-176)sanitize_element(201-203)escape_element(212-214)populate(183-192)get_default(62-64)get_placeholder(84-86)
🪛 PHPMD (2.15.0)
includes/Admin/Settings/Settings.php
80-80: Avoid unused local variables such as '$index'. (Unused Code Rules)
(UnusedLocalVariable)
includes/REST/AdminSettingsController.php
58-58: Avoid unused parameters such as '$request'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (28)
includes/REST/Manager.php (1)
213-213: LGTM! Clean integration of new REST controller.The addition follows the established pattern and correctly maps the file path to the class namespace.
includes/DependencyManagement/Providers/ServiceProvider.php (1)
70-70: LGTM! Proper service provider registration.The addition follows the established pattern for registering service providers and is placed logically with other admin-related providers.
includes/DependencyManagement/Providers/AdminDashboardServiceProvider.php (1)
7-7: LGTM: Proper dependency injection setup.The imports and service registration follow the established pattern correctly, ensuring the Settings page is properly wired with its AdminSettingsProvider dependency and tagged appropriately for the admin dashboard service container.
Also applies to: 12-12, 36-36
includes/Abstracts/Settings.php (2)
44-46: LGTM: Ensures data hydration before population.Good practice to hydrate data before populating to ensure consistency and prevent accessing stale data.
48-56: LGTM: Well-designed convenience method.The
populate_children_only()method provides a clean way to access just the children data while maintaining the same hydration guarantees as the mainpopulate()method.includes/Admin/Settings/TransformerInterface.php (1)
5-35: LGTM: Well-designed transformation interface.The interface provides a clear contract for transformation classes with:
get_target()for identifying transformation typetransform()for data transformation logicset_settings()for configuration- Good documentation and flexible typing to support different transformation needs
The design aligns well with the ElementTransformer implementation shown in the relevant code snippets.
includes/Admin/Settings/Elements/FieldGroup.php (1)
10-51: LGTM: Consistent implementation following established patterns.The FieldGroup class correctly implements the standard settings element pattern with:
- Proper type definition (
'fieldgroup')- Array validation appropriate for group elements
- Standard WordPress sanitization using
wp_unslash()- Simple escape strategy
The implementation is consistent with other element classes like Section, Tab, and SubPage as shown in the relevant code snippets.
includes/REST/AdminSetupGuideController.php (3)
6-6: LGTM: Import addition improves type safety.Adding the
AbstractStepimport enables proper type hinting in the filter documentation.
144-144: LGTM: Enhanced type specificity in documentation.The type hint update from generic
objectto specificAbstractStepimproves the filter documentation accuracy and helps developers understand the expected parameter type.
197-197: LGTM: Method change provides more focused response data.Using
populate_children_only()instead ofpopulate()returns only the children subset after update operations, which aligns with the method's purpose as shown in the relevant code snippets.includes/Admin/Settings/Elements/Fields/Radio.php (2)
24-26: LGTM: Correct validation logic for radio fields.The
data_validationmethod correctly expects a single value rather than an array, which is appropriate for radio button behavior where only one option can be selected.
35-50: LGTM: Appropriate methods for radio field handling.Both
escape_elementandpopulatemethods are correctly implemented:
escape_elementproperly escapes the single value usingesc_attr()populateincludes default value and options which are essential for radio field renderingincludes/Admin/Dashboard/Pages/Settings.php (1)
52-75: LGTM: Clean delegation pattern implementation.The delegation methods (
settings(),scripts(),styles(), andregister()) properly forward calls to the injected settings provider, maintaining clean separation of concerns.includes/Admin/Settings/Elements/SubSection.php (1)
10-51: Consistent implementation following established patterns.The
SubSectionclass correctly follows the same implementation pattern as other element types (FieldGroup,Section,Tab) shown in the relevant code snippets. The validation, sanitization, and escaping methods are consistent across all element types, which enhances maintainability.includes/DependencyManagement/Providers/AdminSettingsServiceProvider.php (1)
8-29: Well-structured service provider implementation.The service provider correctly follows dependency injection patterns with proper service registration and tagging. The implementation is clean and follows established conventions for container service providers.
includes/Admin/Settings/Elements/Fields/Switcher.php (1)
12-94: Well-designed switcher field with fluent interface.The class effectively extends
Radioto provide toggle switch functionality. The fluent interface pattern with method chaining is well-implemented, and thepopulate()method correctly includes both enable and disable states for frontend consumption.includes/Admin/Settings/Elements/SubPage.php (1)
10-51: Consistent implementation following established patterns.The
SubPageclass correctly follows the same implementation pattern as other element types, maintaining consistency across the admin settings element hierarchy.includes/Admin/Settings/Elements/Fields/Select.php (2)
24-26: Data validation logic is correct for single-value select fields.The validation correctly ensures the data is set and is not an array, which is appropriate for select fields that should contain a single selected value.
35-37: Proper escaping implementation for select field values.Using
esc_attr()is appropriate for escaping select field values that will be used in HTML attributes.includes/Admin/Settings/Elements/Fields/Checkbox.php (3)
71-73: Correct validation for checkbox array data.The validation logic properly ensures checkbox data is set and is an array, which is appropriate for handling multiple checkbox selections.
91-98: Well-structured populate method with all necessary data.The method correctly inherits parent data and adds checkbox-specific properties (default, placeholder, options).
56-62: Handle null value parameter in add_option method.The
$valueparameter is typed asstring|nullbut the method doesn't handle the null case, which could lead to unexpected behavior.Either remove null from the type hint or handle the null case:
-public function add_option( string $option, string $value ) { +public function add_option( string $option, ?string $value ) { $this->options[] = array( - 'value' => $value, + 'value' => $value ?? '', 'title' => $option, ); return $this; }Or simplify the type hint:
-public function add_option( string $option, string $value ) { +public function add_option( string $option, string $value ) {Likely an incorrect or invalid review comment.
includes/Admin/Settings/Elements/Fields/MultiCheck.php (1)
111-113: Validation logic is correct for multicheck fields.The validation properly ensures the data is set and is an array, which is appropriate for multiple checkbox selections.
includes/Admin/Settings/Settings.php (1)
108-114: Well-implemented save method with proper data flow.The save method correctly iterates through pages and delegates saving to individual pages when their data is present.
includes/Admin/Settings/Elements/Fields/Commission/CategoryBasedCommission.php (1)
65-67: Validation logic is appropriate for commission data structure.The validation correctly ensures the data is set and is an array, which is suitable for complex commission configuration data.
includes/Admin/Settings/Pages/AbstractPage.php (1)
11-11: No import required for PageInterface
PageInterfacelives in the sameWeDevs\Dokan\Admin\Settings\Pagesnamespace, so PHP resolves it automatically without ausestatement. You can safely ignore the missing import suggestion.Likely an incorrect or invalid review comment.
includes/Admin/Settings/Elements/Fields/Number.php (1)
126-128: WC Dependency Is Explicit—No Guard NeededThe
wc_format_decimal()call insanitize_element()is safe because Dokan-Lite is built as a WooCommerce extension and relies on WooCommerce being active. You don’t need per-method fallbacks for WC functions here.• Dokan’s plugin header already enforces WooCommerce activation.
•wc_format_decimal()is used extensively throughout the codebase without individual guards.includes/Admin/Settings/Elements/Field.php (1)
3-18: Tel class import present—issue resolved.
| public function set_from( $form ): MapperInterface; | ||
| public function get_to(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing documentation and type hints for interface methods.
The interface methods set_from() and get_to() lack proper documentation and type hints, which makes the contract unclear for implementers.
+ /**
+ * Set the source form data for mapping.
+ *
+ * @param mixed $form The form data to map from.
+ * @return MapperInterface Returns self for method chaining.
+ */
- public function set_from( $form ): MapperInterface;
+ public function set_from( $form ): MapperInterface;
+
+ /**
+ * Get the mapped data.
+ *
+ * @return mixed The mapped data.
+ */
- public function get_to();
+ public function get_to();📝 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.
| public function set_from( $form ): MapperInterface; | |
| public function get_to(); | |
| } | |
| /** | |
| * Set the source form data for mapping. | |
| * | |
| * @param mixed $form The form data to map from. | |
| * @return MapperInterface Returns self for method chaining. | |
| */ | |
| public function set_from( $form ): MapperInterface; | |
| /** | |
| * Get the mapped data. | |
| * | |
| * @return mixed The mapped data. | |
| */ | |
| public function get_to(); | |
| } |
🤖 Prompt for AI Agents
In includes/Admin/Settings/MapperInterface.php around lines 13 to 15, the
interface methods set_from() and get_to() lack documentation and type hints. Add
PHPDoc comments above each method describing their purpose, parameters, and
return types. Also, add appropriate type hints to the method signatures to
clarify the expected input and output types for implementers.
| /** | ||
| * Checkbox Field. | ||
| */ |
There was a problem hiding this comment.
Fix class documentation comment.
The comment says "Checkbox Field" but this is a Radio field class.
/**
- * Checkbox Field.
+ * Radio Field.
*/
class Radio extends Checkbox {📝 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.
| /** | |
| * Checkbox Field. | |
| */ | |
| /** | |
| * Radio Field. | |
| */ | |
| class Radio extends Checkbox { | |
| // … | |
| } |
🤖 Prompt for AI Agents
In includes/Admin/Settings/Elements/Fields/Radio.php around lines 5 to 7, update
the class documentation comment to correctly describe the class as a "Radio
Field" instead of "Checkbox Field" to accurately reflect the class purpose.
| * SetupGuide constructor. | ||
| * | ||
| * @param AdminSettingsProvider $admin_setup_guide Admin setup guide instance. | ||
| */ | ||
| public function __construct( AdminSettingsProvider $admin_setup_guide ) { | ||
| $this->settings_provider = $admin_setup_guide; | ||
| } |
There was a problem hiding this comment.
Fix constructor parameter naming inconsistency.
The constructor parameter is named $admin_setup_guide but it should be $settings_provider to match the class property and actual usage.
/**
- * SetupGuide constructor.
+ * Settings constructor.
*
- * @param AdminSettingsProvider $admin_setup_guide Admin setup guide instance.
+ * @param AdminSettingsProvider $settings_provider Admin settings provider instance.
*/
- public function __construct( AdminSettingsProvider $admin_setup_guide ) {
+ public function __construct( AdminSettingsProvider $settings_provider ) {
- $this->settings_provider = $admin_setup_guide;
+ $this->settings_provider = $settings_provider;
}🤖 Prompt for AI Agents
In includes/Admin/Dashboard/Pages/Settings.php around lines 20 to 26, the
constructor parameter is named $admin_setup_guide but the class property is
$settings_provider. Rename the constructor parameter to $settings_provider to
match the property and maintain naming consistency.
| public function __construct( string $id ) { | ||
| $this->id = $id; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constructor should call parent constructor.
The constructor only sets the $id property directly but doesn't call the parent constructor, which may miss important initialization logic from the Field class.
public function __construct( string $id ) {
+ parent::__construct( $id );
- $this->id = $id;
}📝 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.
| public function __construct( string $id ) { | |
| $this->id = $id; | |
| } | |
| public function __construct( string $id ) { | |
| parent::__construct( $id ); | |
| } |
🤖 Prompt for AI Agents
In includes/Admin/Settings/Elements/Fields/MultiCheck.php around lines 39 to 41,
the constructor sets the $id property but does not call the parent constructor.
To fix this, modify the constructor to call parent::__construct($id) to ensure
any initialization logic in the Field base class is executed properly.
| public function data_validation( $data ): bool { | ||
| return isset( $data ) && is_array( $data ); | ||
| } | ||
|
|
||
| /** | ||
| * Populate settings array. | ||
| * | ||
| * @return array | ||
| */ | ||
| public function populate(): array { | ||
| $data = parent::populate(); | ||
| $data['value'] = $this->get_value(); | ||
| $data['default'] = $this->get_default(); | ||
| $data['options'] = $this->get_options(); | ||
|
|
||
| return $data; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing escape_element method implementation.
The class doesn't override the escape_element method from the parent Field class. For array data like MultiCheck, you should implement proper escaping.
Add the missing method:
+/**
+ * Escape data for display.
+ *
+ * @param mixed $data Data for display.
+ *
+ * @return array
+ */
+public function escape_element( $data ): array {
+ if ( ! is_array( $data ) ) {
+ return [];
+ }
+ return array_map( 'esc_attr', $data );
+}
public function populate(): array {📝 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.
| public function data_validation( $data ): bool { | |
| return isset( $data ) && is_array( $data ); | |
| } | |
| /** | |
| * Populate settings array. | |
| * | |
| * @return array | |
| */ | |
| public function populate(): array { | |
| $data = parent::populate(); | |
| $data['value'] = $this->get_value(); | |
| $data['default'] = $this->get_default(); | |
| $data['options'] = $this->get_options(); | |
| return $data; | |
| } | |
| public function data_validation( $data ): bool { | |
| return isset( $data ) && is_array( $data ); | |
| } | |
| /** | |
| * Escape data for display. | |
| * | |
| * @param mixed $data Data for display. | |
| * | |
| * @return array | |
| */ | |
| public function escape_element( $data ): array { | |
| if ( ! is_array( $data ) ) { | |
| return []; | |
| } | |
| return array_map( 'esc_attr', $data ); | |
| } | |
| /** | |
| * Populate settings array. | |
| * | |
| * @return array | |
| */ | |
| public function populate(): array { | |
| $data = parent::populate(); | |
| $data['value'] = $this->get_value(); | |
| $data['default'] = $this->get_default(); | |
| $data['options'] = $this->get_options(); | |
| return $data; | |
| } |
🤖 Prompt for AI Agents
In includes/Admin/Settings/Elements/Fields/MultiCheck.php around lines 111 to
127, the class lacks an override for the escape_element method required for
proper escaping of array data. Implement the escape_element method to handle
escaping each element in the array appropriately, ensuring data is safely
processed and output. This method should iterate over the array elements and
apply escaping to each item, returning the escaped array.
| /** | ||
| * Escape data for display. | ||
| * | ||
| * @param string $data Data for display. | ||
| * | ||
| * @return float | ||
| */ | ||
| public function escape_element( $data ): float { | ||
| return esc_html( $data ); | ||
| } |
There was a problem hiding this comment.
Fix return type mismatch in escape_element method.
The method declares float return type but esc_html() returns a string, causing a type error.
Apply this diff to fix the type mismatch:
/**
* Escape data for display.
*
* @param string $data Data for display.
*
* @return float
*/
-public function escape_element( $data ): float {
- return esc_html( $data );
+public function escape_element( $data ) {
+ return floatval( esc_html( $data ) );
}📝 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.
| /** | |
| * Escape data for display. | |
| * | |
| * @param string $data Data for display. | |
| * | |
| * @return float | |
| */ | |
| public function escape_element( $data ): float { | |
| return esc_html( $data ); | |
| } | |
| /** | |
| * Escape data for display. | |
| * | |
| * @param string $data Data for display. | |
| * | |
| * @return float | |
| */ | |
| public function escape_element( $data ) { | |
| return floatval( esc_html( $data ) ); | |
| } |
🤖 Prompt for AI Agents
In includes/Admin/Settings/Elements/Fields/Number.php around lines 130 to 139,
the escape_element method declares a float return type but returns the result of
esc_html(), which is a string, causing a type mismatch error. To fix this,
change the return type declaration from float to string to match the actual
return value of esc_html().
| /** | ||
| * @var Settings $setup_guide The admin settings manager. | ||
| */ | ||
| $settings = dokan_get_container()->get( Settings::class ); | ||
|
|
There was a problem hiding this comment.
Fix incorrect variable name in PHPDoc.
The PHPDoc refers to $setup_guide but the variable is named $settings.
/**
- * @var Settings $setup_guide The admin settings manager.
+ * @var Settings $settings The admin settings manager.
*/
$settings = dokan_get_container()->get( Settings::class );🤖 Prompt for AI Agents
In includes/REST/AdminSettingsController.php around lines 59 to 63, the PHPDoc
incorrectly names the variable as $setup_guide while the actual variable is
$settings. Update the PHPDoc to refer to $settings instead of $setup_guide to
match the variable name used in the code.
| try { | ||
| /** | ||
| * @var Settings $setup_guide The admin settings manager. | ||
| */ | ||
| $settings_manager = dokan_get_container()->get( Settings::class ); |
There was a problem hiding this comment.
Fix variable name consistency in documentation.
/**
- * @var Settings $setup_guide The admin settings manager.
+ * @var Settings $settings_manager The admin settings manager.
*/
$settings_manager = dokan_get_container()->get( Settings::class );📝 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.
| try { | |
| /** | |
| * @var Settings $setup_guide The admin settings manager. | |
| */ | |
| $settings_manager = dokan_get_container()->get( Settings::class ); | |
| try { | |
| /** | |
| * @var Settings $settings_manager The admin settings manager. | |
| */ | |
| $settings_manager = dokan_get_container()->get( Settings::class ); |
🤖 Prompt for AI Agents
In includes/REST/AdminSettingsController.php around lines 90 to 94, the variable
name in the docblock comment is inconsistent with the actual variable used.
Update the docblock to use the correct variable name $settings_manager instead
of $setup_guide to maintain consistency and clarity.
| /** | ||
| * Filter the admin settings response. | ||
| * | ||
| * Allows modification of the settings response data before it is returned. | ||
| * | ||
| * @since DOKAN_SINCE | ||
| * | ||
| * @param array $settings_array The populated step data. | ||
| */ | ||
| $settings_array = apply_filters( 'dokan_admin_settings_response', $settings_array ); |
There was a problem hiding this comment.
Fix incorrect references to "step data" in documentation.
The documentation mentions "step data" but this controller handles settings data.
/**
* Filter the admin settings response.
*
* Allows modification of the settings response data before it is returned.
*
- * @since DOKAN_SINCE
+ * @since 3.8.0
*
- * @param array $settings_array The populated step data.
+ * @param array $settings_array The populated settings data.
*/📝 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.
| /** | |
| * Filter the admin settings response. | |
| * | |
| * Allows modification of the settings response data before it is returned. | |
| * | |
| * @since DOKAN_SINCE | |
| * | |
| * @param array $settings_array The populated step data. | |
| */ | |
| $settings_array = apply_filters( 'dokan_admin_settings_response', $settings_array ); | |
| /** | |
| * Filter the admin settings response. | |
| * | |
| * Allows modification of the settings response data before it is returned. | |
| * | |
| * @since 3.8.0 | |
| * | |
| * @param array $settings_array The populated settings data. | |
| */ | |
| $settings_array = apply_filters( 'dokan_admin_settings_response', $settings_array ); |
🤖 Prompt for AI Agents
In includes/REST/AdminSettingsController.php around lines 66 to 75, the docblock
incorrectly refers to "step data" instead of "settings data." Update the
documentation comments to replace all mentions of "step data" with "settings
data" to accurately describe the data being handled by this controller.
| private function input_map( string $id, string $input_type ): SettingsElement { | ||
| $class_name = $this->field_map[ $input_type ] ?? $this->field_map['text']; | ||
|
|
||
| /** | ||
| * Filters for setup guide field mapper. | ||
| * | ||
| * @since 4.0.0 | ||
| * | ||
| * @param int $id | ||
| * @param string $input_type | ||
| * @param string $class_name | ||
| */ | ||
| $class_name = apply_filters( 'dokan_admin_setup_guide_field_mapper', $class_name, $id, $input_type ); |
There was a problem hiding this comment.
Fix filter name and documentation inconsistencies.
The filter name references "setup guide" but this is for admin settings. Also, the PHPDoc has incorrect parameter types.
Apply this diff:
/**
- * Filters for setup guide field mapper.
+ * Filters for admin settings field mapper.
*
- * @since 4.0.0
+ * @since DOKAN_SINCE
*
- * @param int $id
+ * @param string $id
* @param string $input_type
* @param string $class_name
*/
- $class_name = apply_filters( 'dokan_admin_setup_guide_field_mapper', $class_name, $id, $input_type );
+ $class_name = apply_filters( 'dokan_admin_settings_field_mapper', $class_name, $id, $input_type );🤖 Prompt for AI Agents
In includes/Admin/Settings/Elements/Field.php around lines 119 to 131, the
filter name 'dokan_admin_setup_guide_field_mapper' and its PHPDoc are
inconsistent with the admin settings context and have incorrect parameter types.
Rename the filter to reflect admin settings context appropriately, such as
'dokan_admin_settings_field_mapper', and update the PHPDoc to correctly document
the parameter types: $id as string and $input_type as string, matching the
method signature.
Introduces a new settings store with actions, reducer, selectors, resolvers, and type definitions. This store handles loading, saving, and updating settings, including dependency management and state tracking for admin settings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
includes/Admin/Settings/Pages/GeneralPage.php (1)
96-96: Fix unused variable in foreach loop.The variable
$page_idis declared but never used within the loop. This creates unnecessary overhead and triggers static analysis warnings.Apply this fix:
- foreach ( $pages as $page_id => $page ) { + foreach ( $pages as $page ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
includes/Admin/Settings/ElementTransformer.php(1 hunks)includes/Admin/Settings/Pages/GeneralPage.php(1 hunks)includes/DependencyManagement/Providers/AdminServiceProvider.php(2 hunks)includes/DependencyManagement/Providers/AdminSettingsServiceProvider.php(1 hunks)src/admin/dashboard/components/Dashboard.tsx(2 hunks)src/admin/dashboard/pages/settings/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/admin/dashboard/pages/settings/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- includes/DependencyManagement/Providers/AdminSettingsServiceProvider.php
- includes/Admin/Settings/ElementTransformer.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
includes/DependencyManagement/Providers/AdminServiceProvider.php (1)
includes/Abstracts/Settings.php (1)
Settings(10-202)
src/admin/dashboard/components/Dashboard.tsx (1)
tests/pw/pages/settingsPage.ts (1)
SettingsPage(12-725)
🪛 PHPMD (2.15.0)
includes/Admin/Settings/Pages/GeneralPage.php
96-96: Avoid unused local variables such as '$page_id'. (Unused Code Rules)
(UnusedLocalVariable)
🔇 Additional comments (7)
src/admin/dashboard/components/Dashboard.tsx (2)
7-7: LGTM! Import follows established patterns.The import statement is properly structured and follows the existing import patterns in the file.
28-32: LGTM! Route configuration is consistent and well-structured.The new settings route follows the same pattern as existing routes (setup, pro-modules) and integrates well with the admin settings restructure. The route ID, path, and element are logical and properly formatted.
includes/Admin/Settings/Pages/GeneralPage.php (3)
9-32: LGTM! Well-structured class with appropriate properties.The class properly extends
AbstractPageand defines clear properties for ID, priority, and storage key. The property visibility and documentation are appropriate.
40-73: LGTM! Methods follow expected patterns.The empty implementations for
register(),scripts(),styles(), andsettings()are appropriate for this base implementation. The method signatures and return types are correct.
80-118: LGTM! Excellent use of design patterns for settings transformation.The
describe_settings()method demonstrates good architecture with:
- Proper use of dependency injection via
dokan_get_container()- Element transformer pattern for legacy settings migration
- Factory pattern for creating sub-page elements
- Fluent interface for method chaining
- Appropriate internationalization with
__()functionThis implementation supports the admin settings restructure effectively.
includes/DependencyManagement/Providers/AdminServiceProvider.php (2)
18-18: LGTM! Settings service properly added to the services array.The addition of
Settings::classto the services array is consistent with the existing pattern used for other services likeStatus::class. This allows theprovides()method to correctly identify that this service provider can provide the Settings service.
5-5: Settings class namespace verified
TheSettingsclass is declared inincludes/Admin/Settings.phpunder theWeDevs\Dokan\Adminnamespace, matching the import inAdminServiceProvider.php. No further changes are required.
Introduces a new settings store in src/stores/settings/store.ts and registers it with @wordpress/data. Also adds a corresponding entry point in webpack-entries.js for 'admin-settings-store' to expose the store on the window object.
…gs-data-store' into feat/admin-settings-with-store
* Add Redux store for settings management Introduces a new settings store with actions, reducer, selectors, resolvers, and type definitions. This store handles loading, saving, and updating settings, including dependency management and state tracking for admin settings. * Add admin settings store and entry point Introduces a new settings store in src/stores/settings/store.ts and registers it with @wordpress/data. Also adds a corresponding entry point in webpack-entries.js for 'admin-settings-store' to expose the store on the window object.
… password, telephone, and currency fields
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/stores/adminSettings/selectors.ts (1)
12-25: Consider simplifying boolean flag selectors.The loading, saving, and needSaving selectors can be simplified by directly returning the state property without destructuring.
getLoading( state: SettingsState ) { - const { loading } = state; - return loading; + return state.loading; }, getSaving( state: SettingsState ) { - const { saving } = state; - return saving; + return state.saving; }, getNeedSaving( state: SettingsState ) { - const { needSaving } = state; - return needSaving; + return state.needSaving; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
includes/Assets.php(1 hunks)src/stores/adminSettings/actions.ts(1 hunks)src/stores/adminSettings/default-state.ts(1 hunks)src/stores/adminSettings/index.ts(1 hunks)src/stores/adminSettings/reducer.ts(1 hunks)src/stores/adminSettings/resolvers.ts(1 hunks)src/stores/adminSettings/selectors.ts(1 hunks)src/stores/adminSettings/store.ts(1 hunks)src/stores/adminSettings/types.ts(1 hunks)webpack-entries.js(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/stores/adminSettings/default-state.ts
- src/stores/adminSettings/index.ts
- src/stores/adminSettings/store.ts
- src/stores/adminSettings/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/stores/adminSettings/selectors.ts (1)
src/stores/adminSettings/types.ts (1)
SettingsState(24-30)
🪛 Biome (1.9.4)
src/stores/adminSettings/reducer.ts
[error] 33-38: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 39-41: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (7)
src/stores/adminSettings/resolvers.ts (1)
1-8: LGTM! Clean resolver implementation.The resolver follows WordPress data store patterns correctly and provides a clean interface for triggering settings retrieval.
webpack-entries.js (1)
78-84: LGTM! Consistent webpack entry configuration.The new admin settings store entry follows the established pattern for other stores and correctly exposes the library under the global
dokan.adminSettingsStorenamespace.includes/Assets.php (1)
646-656: LGTM! Consistent script registration pattern.The admin settings store script registration follows the established pattern used for other stores and includes proper dependency management.
src/stores/adminSettings/reducer.ts (1)
6-76: Well-structured reducer with comprehensive state management.The reducer correctly handles various action types and integrates dependency parsing and application logic. The state updates follow immutable patterns appropriately.
src/stores/adminSettings/selectors.ts (1)
4-10: Excellent flexible selector design.The
getSettingsselector with optional key parameter provides clean access to both individual settings and the complete settings array. The implementation is efficient and follows good practices.src/stores/adminSettings/actions.ts (2)
1-2: LGTM!The imports are correctly structured and necessary for the functionality.
4-34: LGTM!The synchronous action creators follow Redux conventions and are properly typed.
| case 'UPDATE_SETTINGS_VALUE': | ||
| const updatedSettings = [ | ||
| ...settingsElementFinderReplacer( | ||
| [ ...state.settings ], | ||
| action.item | ||
| ), | ||
| ]; | ||
| const updatedDependencies = [ | ||
| ...settingsDependencyParser( [ ...updatedSettings ] ), | ||
| ]; | ||
| return { | ||
| ...state, | ||
| dependencies: [ ...updatedDependencies ], | ||
| settings: [ | ||
| ...settingsDependencyApplicator( | ||
| [ ...updatedSettings ], | ||
| updatedDependencies | ||
| ), | ||
| ], | ||
| needSaving: true, | ||
| }; |
There was a problem hiding this comment.
Wrap variable declarations in block scope to prevent potential access issues.
The static analysis correctly identifies that variables declared in switch cases can be erroneously accessed by other cases. Wrap the declarations in block scopes for safety.
case 'UPDATE_SETTINGS_VALUE':
- const updatedSettings = [
- ...settingsElementFinderReplacer(
- [ ...state.settings ],
- action.item
- ),
- ];
- const updatedDependencies = [
- ...settingsDependencyParser( [ ...updatedSettings ] ),
- ];
- return {
- ...state,
- dependencies: [ ...updatedDependencies ],
- settings: [
- ...settingsDependencyApplicator(
- [ ...updatedSettings ],
- updatedDependencies
- ),
- ],
- needSaving: true,
- };
+ {
+ const updatedSettings = [
+ ...settingsElementFinderReplacer(
+ [ ...state.settings ],
+ action.item
+ ),
+ ];
+ const updatedDependencies = [
+ ...settingsDependencyParser( [ ...updatedSettings ] ),
+ ];
+ return {
+ ...state,
+ dependencies: [ ...updatedDependencies ],
+ settings: [
+ ...settingsDependencyApplicator(
+ [ ...updatedSettings ],
+ updatedDependencies
+ ),
+ ],
+ needSaving: true,
+ };
+ }📝 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.
| case 'UPDATE_SETTINGS_VALUE': | |
| const updatedSettings = [ | |
| ...settingsElementFinderReplacer( | |
| [ ...state.settings ], | |
| action.item | |
| ), | |
| ]; | |
| const updatedDependencies = [ | |
| ...settingsDependencyParser( [ ...updatedSettings ] ), | |
| ]; | |
| return { | |
| ...state, | |
| dependencies: [ ...updatedDependencies ], | |
| settings: [ | |
| ...settingsDependencyApplicator( | |
| [ ...updatedSettings ], | |
| updatedDependencies | |
| ), | |
| ], | |
| needSaving: true, | |
| }; | |
| case 'UPDATE_SETTINGS_VALUE': { | |
| const updatedSettings = [ | |
| ...settingsElementFinderReplacer( | |
| [ ...state.settings ], | |
| action.item | |
| ), | |
| ]; | |
| const updatedDependencies = [ | |
| ...settingsDependencyParser( [ ...updatedSettings ] ), | |
| ]; | |
| return { | |
| ...state, | |
| dependencies: [ ...updatedDependencies ], | |
| settings: [ | |
| ...settingsDependencyApplicator( | |
| [ ...updatedSettings ], | |
| updatedDependencies | |
| ), | |
| ], | |
| needSaving: true, | |
| }; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-38: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 39-41: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In src/stores/adminSettings/reducer.ts between lines 32 and 52, the variables
updatedSettings and updatedDependencies are declared directly inside a switch
case without block scope, which can lead to access issues across cases. To fix
this, wrap the entire case 'UPDATE_SETTINGS_VALUE' code block in curly braces {}
to create a block scope, ensuring these variables are scoped only within this
case and preventing accidental access from other cases.
| fetchSettings() { | ||
| return async ( { dispatch } ) => { | ||
| dispatch( actions.setLoading( true ) ); | ||
| const response = await apiFetch< any >( { | ||
| path: '/dokan/v1/admin/settings', | ||
| } ); | ||
| dispatch( actions.setLoading( false ) ); | ||
| dispatch( actions.setSettings( response ) ); | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Fix type safety and add error handling.
The fetchSettings action has the following issues:
- Type safety: Line 38 uses
anytype, which defeats TypeScript's type checking benefits. - Missing error handling: If the API call fails, the loading state won't be reset.
Apply this diff to fix both issues:
fetchSettings() {
return async ( { dispatch } ) => {
dispatch( actions.setLoading( true ) );
- const response = await apiFetch< any >( {
- path: '/dokan/v1/admin/settings',
- } );
- dispatch( actions.setLoading( false ) );
- dispatch( actions.setSettings( response ) );
+ try {
+ const response = await apiFetch< SettingsElement[] >( {
+ path: '/dokan/v1/admin/settings',
+ } );
+ dispatch( actions.setSettings( response ) );
+ } catch ( error ) {
+ console.error( 'Failed to fetch settings:', error );
+ } finally {
+ dispatch( actions.setLoading( false ) );
+ }
};
},📝 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.
| fetchSettings() { | |
| return async ( { dispatch } ) => { | |
| dispatch( actions.setLoading( true ) ); | |
| const response = await apiFetch< any >( { | |
| path: '/dokan/v1/admin/settings', | |
| } ); | |
| dispatch( actions.setLoading( false ) ); | |
| dispatch( actions.setSettings( response ) ); | |
| }; | |
| }, | |
| fetchSettings() { | |
| return async ( { dispatch } ) => { | |
| dispatch( actions.setLoading( true ) ); | |
| try { | |
| const response = await apiFetch< SettingsElement[] >( { | |
| path: '/dokan/v1/admin/settings', | |
| } ); | |
| dispatch( actions.setSettings( response ) ); | |
| } catch ( error ) { | |
| console.error( 'Failed to fetch settings:', error ); | |
| } finally { | |
| dispatch( actions.setLoading( false ) ); | |
| } | |
| }; | |
| }, |
🤖 Prompt for AI Agents
In src/stores/adminSettings/actions.ts around lines 35 to 44, replace the use of
'any' with a proper type that matches the expected response shape to improve
type safety. Additionally, wrap the API call in a try-catch block to handle
errors; in the catch block, ensure the loading state is reset by dispatching
setLoading(false) and optionally handle or log the error. This will prevent the
loading state from being stuck if the API call fails.
| saveSettings( payload: SettingsElement ) { | ||
| return async ( { dispatch } ) => { | ||
| actions.setSaving( true ); | ||
| const response = await apiFetch< SettingsElement >( { | ||
| path: '/dokan/v1/admin/settings', | ||
| method: 'POST', | ||
| data: payload, | ||
| } ); | ||
|
|
||
| dispatch( actions.setSaving( false ) ); | ||
| dispatch( actions.setNeedSaving( false ) ); | ||
| dispatch( actions.updateSettings( response ) ); | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Fix critical dispatch bug and add error handling.
The saveSettings action has several critical issues:
- Critical bug: Line 47 calls
actions.setSaving(true)directly instead of dispatching it. - Missing error handling: If the API call fails, the saving state won't be reset.
- Type inconsistency: The function accepts a single
SettingsElementbut other actions work with arrays.
Apply this diff to fix the critical issues:
saveSettings( payload: SettingsElement ) {
return async ( { dispatch } ) => {
- actions.setSaving( true );
- const response = await apiFetch< SettingsElement >( {
- path: '/dokan/v1/admin/settings',
- method: 'POST',
- data: payload,
- } );
-
- dispatch( actions.setSaving( false ) );
- dispatch( actions.setNeedSaving( false ) );
- dispatch( actions.updateSettings( response ) );
+ dispatch( actions.setSaving( true ) );
+ try {
+ const response = await apiFetch< SettingsElement >( {
+ path: '/dokan/v1/admin/settings',
+ method: 'POST',
+ data: payload,
+ } );
+ dispatch( actions.setNeedSaving( false ) );
+ dispatch( actions.updateSettings( response ) );
+ } catch ( error ) {
+ console.error( 'Failed to save settings:', error );
+ } finally {
+ dispatch( actions.setSaving( false ) );
+ }
};
},📝 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.
| saveSettings( payload: SettingsElement ) { | |
| return async ( { dispatch } ) => { | |
| actions.setSaving( true ); | |
| const response = await apiFetch< SettingsElement >( { | |
| path: '/dokan/v1/admin/settings', | |
| method: 'POST', | |
| data: payload, | |
| } ); | |
| dispatch( actions.setSaving( false ) ); | |
| dispatch( actions.setNeedSaving( false ) ); | |
| dispatch( actions.updateSettings( response ) ); | |
| }; | |
| }, | |
| saveSettings( payload: SettingsElement ) { | |
| return async ( { dispatch } ) => { | |
| dispatch( actions.setSaving( true ) ); | |
| try { | |
| const response = await apiFetch< SettingsElement >( { | |
| path: '/dokan/v1/admin/settings', | |
| method: 'POST', | |
| data: payload, | |
| } ); | |
| dispatch( actions.setNeedSaving( false ) ); | |
| dispatch( actions.updateSettings( response ) ); | |
| } catch ( error ) { | |
| console.error( 'Failed to save settings:', error ); | |
| } finally { | |
| dispatch( actions.setSaving( false ) ); | |
| } | |
| }; | |
| }, |
🤖 Prompt for AI Agents
In src/stores/adminSettings/actions.ts around lines 45 to 58, fix the critical
bug by dispatching actions.setSaving(true) instead of calling it directly. Add a
try-catch block around the API call to handle errors and ensure
dispatch(actions.setSaving(false)) is called in both success and failure cases
to reset the saving state. Also, update the function parameter to accept an
array of SettingsElement to maintain type consistency with other actions.
…min-settings # Conflicts: # src/admin/dashboard/pages/settings/index.tsx
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (1)
src/admin/dashboard/pages/settings/Elements/FiendGroup.tsx (1)
1-46: Fix the filename typo.The filename has a typo: "FiendGroup.tsx" should be "FieldGroup.tsx". This affects imports in other files and could cause confusion.
Rename the file from
FiendGroup.tsxtoFieldGroup.tsxand update the import inSettingsParser.tsx:-import FieldGroup from './FiendGroup'; +import FieldGroup from './FieldGroup';
♻️ Duplicate comments (4)
src/admin/dashboard/pages/settings/Elements/Section.tsx (1)
1-1: Fix the import path for better organization.Same issue as in Password component - importing from setup guide directory is incorrect.
src/admin/dashboard/pages/settings/Elements/FiendGroup.tsx (2)
1-1: Fix the import path for better organization.Same import path issue as other components.
28-28: Add null check for element.children.Same issue as in Section component - missing null check before mapping.
-{ element.children.map( ( child ) => { +{ element.children?.map( ( child ) => {src/admin/dashboard/pages/settings/Elements/SettingsParser.tsx (1)
1-1: Fix the import path for better organization.Same import path issue as other components.
🧹 Nitpick comments (21)
src/admin/dashboard/pages/settings/Elements/Fields/Password.tsx (2)
11-11: Add type annotation for the event parameter.The event parameter lacks proper TypeScript typing, which reduces type safety.
-const handleValueChange = ( event ) => { +const handleValueChange = ( event: React.ChangeEvent<HTMLInputElement> ) => {
33-33: Reconsider the autocomplete setting for password fields.Using
autoComplete="off"for password fields is generally not recommended as it prevents browsers from offering to save passwords, which reduces user experience.-autoComplete="off" +autoComplete="current-password"src/admin/dashboard/pages/settings/Elements/SettingsParser.tsx (1)
53-53: Remove unnecessary Fragment.The static analysis tool correctly identified an unnecessary Fragment that should be replaced with null.
-<></>, +null,src/admin/dashboard/pages/settings/index.tsx (3)
197-198: Fix text domain inconsistency.The component uses
'dokan-driver'text domain instead of'dokan-lite'which is used elsewhere in the same file.-{ isSaving - ? __( 'Saving..', 'dokan-driver' ) - : __( 'Save', 'dokan-driver' ) } +{ isSaving + ? __( 'Saving..', 'dokan-lite' ) + : __( 'Save', 'dokan-lite' ) }
133-138: Implement proper error handling and user feedback.The TODOs indicate missing success and error feedback for the save operation. This creates poor user experience.
Do you want me to help implement proper toast notifications or alert messages for save operations?
.then( () => { setIsSaving( false ); - // TODO: Say updated. + // Show success notification + dispatch( 'core/notices' ).createSuccessNotice( + __( 'Settings saved successfully!', 'dokan-lite' ) + ); } ) .catch( ( err ) => { setIsSaving( false ); - // TODO: Say Error. + // Show error notification + dispatch( 'core/notices' ).createErrorNotice( + __( 'Failed to save settings. Please try again.', 'dokan-lite' ) + ); } );
84-109: Consider simplifying the complex conditional logic.The nested conditional logic for determining elements is complex and could be refactored for better readability and maintainability.
Consider extracting this logic into a separate function:
const determineElements = useCallback(() => { if (loading) return []; if (!pages?.length && !tabs?.length) { return allSettings; } if (pages?.length && !tabs?.length && selectedPage) { return pages.find(child => child.id === selectedPage)?.children || []; } if (!pages?.length && tabs?.length && selectedTab) { return tabs.find(child => child.id === selectedTab)?.children || []; } if (pages?.length && tabs?.length && selectedPage && selectedTab) { return tabs.find(child => child.id === selectedTab)?.children || []; } return []; }, [allSettings, pages, selectedPage, tabs, selectedTab, loading]); useEffect(() => { setElements(determineElements()); }, [determineElements]);src/admin/dashboard/pages/settings/Elements/Fields/FieldParser.tsx (1)
20-20: Address the TODO comment for custom field support.The TODO indicates missing support for custom input fields and hooks. This could limit extensibility of the settings system.
Would you like me to help design an implementation for custom field support or create an issue to track this enhancement?
src/admin/dashboard/pages/settings/Elements/Fields/Tel.tsx (2)
30-30: Consider making the phone pattern more flexible for international numbers.The current pattern
[0-9]{3}-[0-9]{2}-[0-9]{3}is very restrictive and may not accommodate international phone number formats.Consider using a more flexible pattern or making it configurable:
- pattern="[0-9]{3}-[0-9]{2}-[0-9]{3}" + pattern={element.pattern || "[0-9+\\-\\s()]+"}
11-11: Add TypeScript type annotation for the event parameter.The
eventparameter should have proper TypeScript typing for better type safety.- const handleValueChange = ( event ) => { + const handleValueChange = ( event: React.ChangeEvent<HTMLInputElement> ) => {src/admin/dashboard/pages/settings/Elements/Fields/MultiCheck.tsx (1)
13-13: Add TypeScript type annotation for the values parameter.The
valuesparameter should have proper TypeScript typing.- const handleChange = ( values ) => { + const handleChange = ( values: string[] ) => {src/admin/dashboard/pages/settings/Elements/Tab.tsx (2)
16-16: Make the onTabClick parameter type more specific.The parameter name
tabsis misleading - it should reflect that it receives a tab ID.- onTabClick: ( tabs ) => void; + onTabClick: ( tabId: string ) => void;
35-35: Use null instead of empty fragments for better performance.Returning
nullis more semantically correct and slightly more performant than empty fragments for conditional rendering.- return <></>; + return null;Also applies to: 51-51
src/admin/dashboard/pages/settings/Elements/Fields/Text.tsx (1)
11-11: Add TypeScript type annotation for the event parameter.The
eventparameter should have proper TypeScript typing for better type safety.- const handleValueChange = ( event ) => { + const handleValueChange = ( event: React.ChangeEvent<HTMLInputElement> ) => {src/admin/dashboard/pages/settings/Elements/Fields/Switcher.tsx (1)
9-18: Simplify the initial state determination logic.The current logic for determining
initialEnabledis verbose and repetitive. Consider refactoring for better readability and maintainability.- let initialEnabled = false; - if ( element.value === enableState?.value ) { - initialEnabled = true; - } else if ( element.value === disableState?.value ) { - initialEnabled = false; - } else if ( element.default === enableState?.value ) { - initialEnabled = true; - } else if ( element.default === disableState?.value ) { - initialEnabled = false; - } + const currentValue = element.value ?? element.default; + const initialEnabled = currentValue === enableState?.value;src/admin/dashboard/pages/settings/Elements/SubSection.tsx (1)
19-19: Consider making the column span configurable.The hardcoded
col-span-4class may limit layout flexibility. Consider making this configurable through props or element configuration.- <div className="col-span-4"> + <div className={`col-span-${element.colSpan || 4}`}>src/admin/dashboard/pages/settings/Elements/Fields/Commission/CombineInput.tsx (1)
11-11: Use consistent return pattern for conditional rendering.This component returns
nullwhile other components in the codebase return<></>for the same condition. Consider maintaining consistency.- return null; + return <></>;src/admin/dashboard/pages/settings/Elements/Menu.tsx (1)
3-5: Consider using a dedicated utility library for class name concatenation.While the custom
classNamesfunction works, consider using a more robust library likeclsxfor better performance and features.+import { clsx } from 'clsx'; -function classNames( ...classes ) { - return classes.filter( Boolean ).join( ' ' ); -}Then replace
classNameswithclsxin the component.src/admin/dashboard/pages/settings/Elements/Fields/Radio.tsx (1)
13-13: Add type annotation for better type safetyThe
newValueparameter should have a type annotation to ensure type safety.- const handleChange = ( newValue ) => { + const handleChange = ( newValue: string ) => {src/admin/dashboard/pages/settings/Elements/Fields/RadioBox.tsx (1)
17-17: Add type annotations for event handlersThe
handleChangeandrenderIconfunctions need type annotations for better type safety.- const handleChange = ( newValue ) => { + const handleChange = ( newValue: string ) => {And for
renderIcon:- const renderIcon = ( value ) => { + const renderIcon = ( value: string ) => {Also applies to: 26-26
src/admin/dashboard/pages/settings/Elements/Fields/Select.tsx (1)
13-13: Add type annotation for change handlerThe
onHandleChangefunction parameter should have a type annotation.- const onHandleChange = ( value ) => { + const onHandleChange = ( value: string ) => {src/admin/dashboard/pages/settings/Elements/Fields/Commission/CategoryBasedCommission.tsx (1)
251-286: Replace custom isEqual with a library functionThe custom deep equality implementation could be replaced with a well-tested library function for better reliability and maintainability.
Consider using lodash's
isEqual:import { isEqual } from 'lodash'; // Remove the custom implementation and use: if (isEqual(newCommission?.items[key], newCommission?.all)) { delete newCommission?.items[key]; }Or if you prefer not to add a dependency, at least simplify the implementation using modern JavaScript:
const isEqual = (a: any, b: any): boolean => { return JSON.stringify(a) === JSON.stringify(b); };Note: The JSON approach works for simple objects but has limitations with circular references and non-serializable values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
includes/Admin/Settings/Pages/AbstractPage.php(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/Commission/CategoryBasedCommission.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/Commission/CombineInput.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/Currency.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/DokanCurrencyInput.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/FieldParser.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/MultiCheck.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/Password.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/Radio.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/RadioBox.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/Select.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/SelectorCard.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/Switcher.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/Tel.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Fields/Text.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/FiendGroup.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Menu.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Section.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/SettingsParser.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/SubSection.tsx(1 hunks)src/admin/dashboard/pages/settings/Elements/Tab.tsx(1 hunks)src/admin/dashboard/pages/settings/components/icons/AdminIcon.tsx(1 hunks)src/admin/dashboard/pages/settings/components/icons/MarkedChecked.tsx(1 hunks)src/admin/dashboard/pages/settings/components/icons/UnMarkedChecked.tsx(1 hunks)src/admin/dashboard/pages/settings/components/icons/VendorIcon.tsx(1 hunks)src/admin/dashboard/pages/settings/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- src/admin/dashboard/pages/settings/components/icons/UnMarkedChecked.tsx
- src/admin/dashboard/pages/settings/components/icons/AdminIcon.tsx
- src/admin/dashboard/pages/settings/components/icons/VendorIcon.tsx
- src/admin/dashboard/pages/settings/components/icons/MarkedChecked.tsx
- src/admin/dashboard/pages/settings/Elements/Fields/SelectorCard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/Admin/Settings/Pages/AbstractPage.php
🧰 Additional context used
🧬 Code Graph Analysis (11)
src/admin/dashboard/pages/settings/Elements/SettingsParser.tsx (1)
src/admin/dashboard/pages/setup-guide/StepSettings.tsx (1)
SettingsProps(60-64)
src/admin/dashboard/pages/settings/index.tsx (2)
tests/pw/pages/settingsPage.ts (1)
SettingsPage(12-725)includes/Admin/Menu.php (1)
Menu(7-114)
src/admin/dashboard/pages/settings/Elements/Fields/MultiCheck.tsx (1)
src/admin/dashboard/pages/setup-guide/StepSettings.tsx (1)
SettingsProps(60-64)
src/admin/dashboard/pages/settings/Elements/Section.tsx (1)
src/admin/dashboard/pages/setup-guide/StepSettings.tsx (1)
SettingsProps(60-64)
src/admin/dashboard/pages/settings/Elements/Fields/Text.tsx (1)
src/admin/dashboard/pages/setup-guide/StepSettings.tsx (1)
SettingsProps(60-64)
src/admin/dashboard/pages/settings/Elements/Fields/Select.tsx (1)
src/admin/dashboard/pages/setup-guide/StepSettings.tsx (1)
SettingsProps(60-64)
src/admin/dashboard/pages/settings/Elements/Fields/FieldParser.tsx (1)
src/admin/dashboard/pages/setup-guide/StepSettings.tsx (1)
SettingsProps(60-64)
src/admin/dashboard/pages/settings/Elements/Fields/RadioBox.tsx (1)
src/admin/dashboard/pages/setup-guide/StepSettings.tsx (1)
SettingsProps(60-64)
src/admin/dashboard/pages/settings/Elements/FiendGroup.tsx (1)
src/admin/dashboard/pages/setup-guide/StepSettings.tsx (1)
SettingsProps(60-64)
src/admin/dashboard/pages/settings/Elements/Fields/Password.tsx (1)
src/admin/dashboard/pages/setup-guide/StepSettings.tsx (1)
SettingsProps(60-64)
src/admin/dashboard/pages/settings/Elements/Fields/Tel.tsx (1)
src/admin/dashboard/pages/setup-guide/StepSettings.tsx (1)
SettingsProps(60-64)
🪛 Biome (1.9.4)
src/admin/dashboard/pages/settings/Elements/SettingsParser.tsx
[error] 53-53: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
(lint/complexity/noUselessFragments)
src/admin/dashboard/pages/settings/Elements/Fields/FieldParser.tsx
[error] 121-121: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 122-122: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 123-123: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 124-124: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 125-125: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 126-126: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 127-127: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 128-128: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 129-129: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 130-130: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 131-131: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 132-132: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 133-133: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 134-134: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 135-135: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
src/admin/dashboard/pages/settings/Elements/Fields/Commission/CategoryBasedCommission.tsx
[error] 106-106: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 129-129: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 464-464: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (11)
src/admin/dashboard/pages/settings/Elements/Section.tsx (1)
26-26: Verify HTML sanitization in Settings Section componentI wasn’t able to find any evidence of server-side sanitization for the
titleanddescriptionfields before they’re rendered via<RawHTML>. Please ensure these values are cleaned (e.g. with wp_kses, sanitize_text_field, or a similar whitelist) in your REST endpoint or PHP before sending to the frontend.• File to audit:
- src/admin/dashboard/pages/settings/Elements/Section.tsx (around line 26)
- Also applies to its SubSection counterpart
src/admin/dashboard/pages/settings/Elements/Fields/FieldParser.tsx (2)
121-136: The fallthrough cases are intentional documentation.The static analysis tool is incorrectly flagging these cases as useless. These cases serve as documentation for supported HTML input types that should use the Text component as fallback.
The fallthrough pattern clearly documents which HTML input types are supported and intentionally handled by the default case.
137-148: Well-designed extensibility with WordPress filters.The use of
applyFiltersprovides a clean extensibility mechanism for third-party developers to customize field rendering.src/admin/dashboard/pages/settings/Elements/Fields/Tel.tsx (1)
4-4: UnusedgetSettingprop from SettingsProps interface.The component receives
getSettingfrom SettingsProps but doesn't use it. Consider if this indicates missing functionality or if the prop should be omitted.src/admin/dashboard/pages/settings/Elements/Fields/MultiCheck.tsx (1)
21-21: Verify the initialValue calculation logic.The use of
Object.values()onelement.value || element.defaultmay not produce the expected array format for checkbox values.Please verify that this calculation produces the correct format expected by
SimpleCheckboxGroup. Consider if the values should be an array of strings rather than object values.src/admin/dashboard/pages/settings/Elements/Fields/Text.tsx (2)
4-4: UnusedgetSettingprop from SettingsProps interface.The component receives
getSettingfrom SettingsProps but doesn't use it. This pattern is consistent across multiple field components.
19-41: Well-structured text input component with good accessibility.The component properly implements controlled input with accessibility attributes, RawHTML support for titles/descriptions, and consistent styling.
src/admin/dashboard/pages/settings/Elements/SubSection.tsx (1)
14-16: Well-implemented child type detection and rendering.The logic for detecting when all children are fields and applying appropriate styling is clean and efficient. The mapping implementation correctly passes through necessary props.
Also applies to: 39-50
src/admin/dashboard/pages/settings/Elements/Fields/Currency.tsx (1)
13-26: Excellent input validation and error handling.The validation logic properly handles negative values and provides user feedback. The state management approach with separate error state is well-implemented.
src/admin/dashboard/pages/settings/Elements/Menu.tsx (1)
24-49: Excellent accessibility and event handling implementation.The component properly implements:
- Semantic navigation structure
- Accessible
aria-currentattributes- Proper event handling with
preventDefault- Good conditional styling for active states
src/admin/dashboard/pages/settings/Elements/Fields/Commission/CategoryBasedCommission.tsx (1)
464-467: ReviewdangerouslySetInnerHTMLusage for item.name
UsingdangerouslySetInnerHTMLwith unsanitizeditem.namecan open the door to XSS if any category name contains HTML/script.• File: src/admin/dashboard/pages/settings/Elements/Fields/Commission/CategoryBasedCommission.tsx
• Lines: around 464–467Suggested fixes:
- <span - title={ item.name } - dangerouslySetInnerHTML={{ __html: item.name }} - ></span> + <span title={ item.name }> + { item.name } + </span>If you must render HTML in category names, sanitize first (e.g. DOMPurify):
import DOMPurify from 'dompurify'; // … dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(item.name) }}Please verify whether
item.nameever includes HTML that needs rendering and ensure a sanitization strategy is in place.
| @@ -0,0 +1,41 @@ | |||
| import { useState } from '@wordpress/element'; | |||
| import { SettingsProps } from '../../StepSettings'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the import path for better organization.
Importing SettingsProps from StepSettings in a setup guide directory seems incorrect for a settings field component. This suggests the interface should be moved to a shared location or redefined locally.
Consider moving the interface to a shared types file:
-import { SettingsProps } from '../../StepSettings';
+import { SettingsProps } from '../../../types/SettingsTypes';📝 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.
| import { SettingsProps } from '../../StepSettings'; | |
| -import { SettingsProps } from '../../StepSettings'; | |
| +import { SettingsProps } from '../../../types/SettingsTypes'; |
🤖 Prompt for AI Agents
In src/admin/dashboard/pages/settings/Elements/Fields/Password.tsx at line 2,
the import path for SettingsProps is incorrect as it points to StepSettings,
which is not appropriate for a settings field component. Move the SettingsProps
interface to a shared types file accessible by multiple components, then update
the import statement in Password.tsx to import SettingsProps from this new
shared location to improve code organization and maintainability.
| : '' | ||
| }` } | ||
| > | ||
| { element.children.map( ( child ) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for element.children.
The code assumes element.children always exists and is an array, which could cause runtime errors.
-{ element.children.map( ( child ) => {
+{ element.children?.map( ( child ) => {📝 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.
| { element.children.map( ( child ) => { | |
| { element.children?.map( ( child ) => { |
🤖 Prompt for AI Agents
In src/admin/dashboard/pages/settings/Elements/Section.tsx at line 39, add a
null check for element.children before mapping over it to prevent runtime errors
if element.children is undefined or null. Modify the code to only call map if
element.children exists and is an array, for example by using optional chaining
or a conditional check.
| import Section from './Section'; | ||
| import SubSection from './SubSection'; | ||
| import FieldParser from './Fields/FieldParser'; | ||
| import FieldGroup from './FiendGroup'; |
There was a problem hiding this comment.
Fix the import from the misspelled filename.
The import references the misspelled filename "FiendGroup" instead of "FieldGroup".
-import FieldGroup from './FiendGroup';
+import FieldGroup from './FieldGroup';📝 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.
| import FieldGroup from './FiendGroup'; | |
| import FieldGroup from './FieldGroup'; |
🤖 Prompt for AI Agents
In src/admin/dashboard/pages/settings/Elements/SettingsParser.tsx at line 5, the
import statement incorrectly references "FiendGroup" instead of the correct
filename "FieldGroup". Update the import path to './FieldGroup' to fix the typo
and correctly import the module.
| <SettingsParser | ||
| key={ | ||
| element.hook_key + | ||
| '-settings-parser' | ||
| } | ||
| element={ element } | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Add missing required props to SettingsParser.
The SettingsParser component expects getSetting and onValueChange props according to the SettingsProps interface, but they're not being passed.
<SettingsParser
key={
element.hook_key +
'-settings-parser'
}
element={ element }
+ getSetting={ getSetting }
+ onValueChange={ onValueChange }
/>You'll need to implement these functions in the SettingsPage component:
const getSetting = (id: string): SettingsElement | undefined => {
return allSettings?.find(setting => setting.id === id);
};
const onValueChange = (updatedElement: SettingsElement) => {
dispatch(settingsStore).updateSetting(updatedElement);
};🤖 Prompt for AI Agents
In src/admin/dashboard/pages/settings/index.tsx around lines 173 to 180, the
SettingsParser component is missing the required props getSetting and
onValueChange as defined in the SettingsProps interface. To fix this, implement
the getSetting function to find a setting by id from allSettings, and the
onValueChange function to dispatch an updateSetting action with the updated
element. Then pass these two functions as props to the SettingsParser component
where it is rendered.
| import { SettingsProps } from '../../StepSettings'; | ||
|
|
||
| const MultiCheck = ( { element, onValueChange }: SettingsProps ) => { | ||
| const [ selectedValues, setSelectedValues ] = useState( [] ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize state with element's current value.
The selectedValues state is initialized as an empty array, but it should reflect the current element value to maintain consistency.
- const [ selectedValues, setSelectedValues ] = useState( [] );
+ const [ selectedValues, setSelectedValues ] = useState(
+ Object.values( element.value || element.default || {} )
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [ selectedValues, setSelectedValues ] = useState( [] ); | |
| const [ selectedValues, setSelectedValues ] = useState( | |
| Object.values( element.value || element.default || {} ) | |
| ); |
🤖 Prompt for AI Agents
In src/admin/dashboard/pages/settings/Elements/Fields/MultiCheck.tsx at line 6,
the selectedValues state is initialized as an empty array, which does not
reflect the element's current value. Update the useState initialization to use
the element's current value instead of an empty array to ensure the state is
consistent with the element's data.
| value?: string; | ||
| } | ||
|
|
||
| export const formatNumber = ( value ) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type annotations for utility functions
Both formatNumber and unformatNumber functions are missing type annotations for their parameters.
-export const formatNumber = ( value ) => {
+export const formatNumber = ( value: string | number ) => {And for unformatNumber:
-const unformatNumber = ( value ) => {
+const unformatNumber = ( value: string | number ) => {Also applies to: 39-39
🤖 Prompt for AI Agents
In src/admin/dashboard/pages/settings/Elements/Fields/DokanCurrencyInput.tsx at
lines 15 and 39, the utility functions formatNumber and unformatNumber lack type
annotations for their parameters. Add appropriate TypeScript type annotations to
the parameters of both functions to specify the expected input types, improving
type safety and code clarity.
| return window.accounting.formatNumber( | ||
| value, | ||
| // @ts-ignore | ||
| window?.dokanWithdrawDashboard?.currency.precision, | ||
| window?.dokanWithdrawDashboard?.currency.thousand, | ||
| window?.dokanWithdrawDashboard?.currency.decimal | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove @ts-ignore and properly type window extensions
Using @ts-ignore hides type errors and should be avoided. Define proper types for the window extensions.
Create a type declaration file or add to an existing one:
declare global {
interface Window {
accounting?: {
formatNumber: (value: number | string, precision?: number, thousand?: string, decimal?: string) => string;
unformat: (value: string, decimal?: string) => number;
};
dokanWithdrawDashboard?: {
currency: {
precision?: number;
thousand?: string;
decimal?: string;
symbol?: string;
position?: 'left' | 'left_space' | 'right' | 'right_space';
};
};
}
}Then remove the @ts-ignore comment:
return window.accounting.formatNumber(
value,
- // @ts-ignore
window?.dokanWithdrawDashboard?.currency.precision,
window?.dokanWithdrawDashboard?.currency.thousand,
window?.dokanWithdrawDashboard?.currency.decimal
);🤖 Prompt for AI Agents
In src/admin/dashboard/pages/settings/Elements/Fields/DokanCurrencyInput.tsx
around lines 30 to 36, remove the @ts-ignore comment and properly type the
window extensions by adding a global declaration for the Window interface.
Define the accounting and dokanWithdrawDashboard properties with their
respective types as shown in the comment, ensuring TypeScript recognizes these
properties without errors. This can be done by creating or updating a type
declaration file with the provided interface extension.
| return null; | ||
| } | ||
|
|
||
| const { currency } = adminWithdrawData; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type the global adminWithdrawData object
Accessing global variables without proper typing reduces type safety.
Add proper type declaration:
declare global {
const adminWithdrawData: {
currency: {
symbol?: string;
precision?: number;
thousand?: string;
decimal?: string;
};
};
}🤖 Prompt for AI Agents
In
src/admin/dashboard/pages/settings/Elements/Fields/Commission/CategoryBasedCommission.tsx
at line 16, the global variable adminWithdrawData is accessed without a proper
type declaration, reducing type safety. Add a global type declaration for
adminWithdrawData specifying its structure, including the currency object with
optional properties symbol, precision, thousand, and decimal, to ensure type
safety when accessing this variable.
| const { currency } = adminWithdrawData; | ||
| const getCurrencySymbol = currency?.symbol; | ||
|
|
||
| const getCategories = ( categoriesData ) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type annotations to helper functions
Multiple helper functions lack type annotations, reducing type safety and code maintainability.
// Add proper type definitions
interface Category {
term_id: string;
parent_id: string;
parents: number[];
children: any[];
name: string;
}
interface CommissionData {
all?: { percentage: string; flat: string };
items?: Record<string, { percentage: string; flat: string }>;
}
// Then update function signatures:
const getCategories = (categoriesData: Record<string, Category>): Category[] => {
const catRowClick = (item: Category): void => {
const getChildren = (parentId: string): string[] => {
const showCatRow = (item: Category): boolean => {
const getCommissionValue = (commissionType: 'percentage' | 'flat', termId: string): string => {
const unFormatValue = (value: string): string => {
const formatValue = (value: string | number): string => {
const validatePercentage = (percentage: string): string => {Also applies to: 64-64, 87-87, 97-97, 105-105, 210-210, 224-224, 237-237
🤖 Prompt for AI Agents
In
src/admin/dashboard/pages/settings/Elements/Fields/Commission/CategoryBasedCommission.tsx
at lines 19, 64, 87, 97, 105, 210, 224, and 237, several helper functions lack
type annotations, which reduces type safety and maintainability. Add appropriate
TypeScript type annotations to these functions based on the provided interfaces
Category and CommissionData, and specify parameter and return types as indicated
in the comment to improve code clarity and robustness.
| }; | ||
|
|
||
| const getCommissionValue = ( commissionType, termId ) => { | ||
| if ( commission?.items?.hasOwnProperty( termId ) ) { |
There was a problem hiding this comment.
Use Object.hasOwn instead of hasOwnProperty
Direct use of hasOwnProperty can be problematic if the object has a property with that name. Use Object.hasOwn for better safety.
- if ( commission?.items?.hasOwnProperty( termId ) ) {
+ if ( Object.hasOwn( commission?.items || {}, termId ) ) {And on line 129:
- if ( commissions.hasOwnProperty( termId ) ) {
+ if ( Object.hasOwn( commissions, termId ) ) {Also applies to: 129-129
🧰 Tools
🪛 Biome (1.9.4)
[error] 106-106: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🤖 Prompt for AI Agents
In
src/admin/dashboard/pages/settings/Elements/Fields/Commission/CategoryBasedCommission.tsx
at lines 106 and 129, replace the use of
`commission?.items?.hasOwnProperty(termId)` with
`Object.hasOwn(commission?.items, termId)` to avoid issues when the object has a
property named `hasOwnProperty`. This change ensures safer property existence
checks by using the static method `Object.hasOwn`.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/admin/dashboard/pages/settings/Elements/Menu.tsx (3)
9-11: Add TypeScript typing to theclassNamesutility function.For better type safety in a TypeScript project, explicitly type the parameters.
-function classNames( ...classes ) { +function classNames( ...classes: (string | boolean | undefined)[] ): string { return classes.filter( Boolean ).join( ' ' ); }
30-33: Filter non-displayed items before mapping.Instead of returning empty fragments, filter the array first for better performance and cleaner code.
- { ! loading && - pages.map( ( item ) => { - if ( ! item.display ) { - return <></>; - } + { ! loading && + pages.filter( item => item.display ).map( ( item ) => {
85-98: Consider using semantic href values or button elements.The
hrefis set tosubItem.idwhich may not be a valid URL. Since the click handler prevents default navigation, consider using#as href or replacing with a button element.<DisclosureButton as="a" - href={ - subItem.id - } + href="#" onClick={ ( e ) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/admin/dashboard/pages/settings/Elements/Menu.tsx(1 hunks)src/admin/dashboard/pages/settings/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/admin/dashboard/pages/settings/index.tsx
| <li | ||
| key={ | ||
| subItem.title | ||
| } | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use id instead of title as the React key.
Using title as a key could cause issues if titles are not unique. Use the id property instead.
{ item?.children?.map(
( subItem ) => (
<li
- key={
- subItem.title
- }
+ key={ subItem.id }
>📝 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.
| <li | |
| key={ | |
| subItem.title | |
| } | |
| > | |
| <li | |
| key={ subItem.id } | |
| > |
🤖 Prompt for AI Agents
In src/admin/dashboard/pages/settings/Elements/Menu.tsx between lines 80 and 84,
replace the React key from using subItem.title to subItem.id to ensure
uniqueness and avoid potential rendering issues. Update the key attribute in the
<li> element to use subItem.id instead of subItem.title.
| onMenuClick, | ||
| }: { | ||
| pages: SettingsElement[]; | ||
| parentPages?: SettingsElement[]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused parentPages prop.
The parentPages prop is declared in the interface but never used in the component implementation.
}: {
pages: SettingsElement[];
- parentPages?: SettingsElement[];
loading: boolean;
activePage: string;
onMenuClick: ( page: string ) => void;📝 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.
| parentPages?: SettingsElement[]; | |
| }: { | |
| pages: SettingsElement[]; | |
| loading: boolean; | |
| activePage: string; | |
| onMenuClick: ( page: string ) => void; |
🤖 Prompt for AI Agents
In src/admin/dashboard/pages/settings/Elements/Menu.tsx at line 20, the prop
`parentPages` is declared in the interface but not used anywhere in the
component. Remove the `parentPages` declaration from the interface to clean up
unused code and avoid confusion.
| { ! item.children ? ( | ||
| <a | ||
| href={ item.id } | ||
| className={ classNames( | ||
| item.id === activePage || | ||
| item.children.some( | ||
| ( child ) => | ||
| child.id === | ||
| activePage | ||
| ) | ||
| ? 'bg-gray-50' | ||
| : 'hover:bg-gray-50', | ||
| 'block rounded-md py-2 pl-10 pr-2 text-sm/6 font-semibold text-gray-700' | ||
| ) } | ||
| > | ||
| { item.title } | ||
| </a> | ||
| ) : ( |
There was a problem hiding this comment.
Fix critical runtime error when accessing children.
The code checks !item.children but then tries to call item.children.some() in the same branch, which will throw a TypeError.
{ ! item.children ? (
<a
href={ item.id }
className={ classNames(
- item.id === activePage ||
- item.children.some(
- ( child ) =>
- child.id ===
- activePage
- )
+ item.id === activePage
? 'bg-gray-50'
: 'hover:bg-gray-50',
'block rounded-md py-2 pl-10 pr-2 text-sm/6 font-semibold text-gray-700'
) }
>
{ item.title }
</a>🤖 Prompt for AI Agents
In src/admin/dashboard/pages/settings/Elements/Menu.tsx between lines 36 and 53,
the code checks if item.children is falsy but then calls item.children.some() in
the same condition, causing a runtime error. Fix this by restructuring the
condition to only call item.children.some() when item.children exists and is an
array, ensuring no method is called on undefined or null.
Introduces the updateSettingsValue action to handle updating individual settings elements in the adminSettings store.
# Conflicts: # includes/DependencyManagement/Providers/ServiceProvider.php # src/admin/dashboard/components/Dashboard.tsx
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
New Features
Improvements
Documentation