Integration branch: Sidebar Metabox - #1332
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Gutenberg sidebar and issue-modal feature set: new REST endpoints for sidebar data and dismissing issues, server-side data-assembly helpers, React sidebar and modal UI with a WP data store, admin enqueue/editor gating, styling, build config, database column, and PHPUnit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Sidebar as Sidebar UI
participant Store as WP Data Store
participant REST as REST API
participant DB as Database
User->>Sidebar: Open/Edit post
Sidebar->>Store: set postId / subscribe
Store->>REST: GET /accessibility-checker/v1/sidebar-data/{postId}
REST->>DB: query summaries, rule details, readability
DB-->>REST: compiled sidebar payload
REST-->>Store: WP_REST_Response (data)
Store-->>Sidebar: setData (errors,warnings,readability)
Sidebar->>User: render panels
User->>Sidebar: Dismiss/Restore issue
Sidebar->>REST: POST /accessibility-checker/v1/dismiss-issue/{issueId}
REST->>DB: update ignore state/fields
DB-->>REST: result
REST->>Sidebar: response + dispatch edac-ignore-updated event
Sidebar->>Store: refetchData -> update UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
Summary of ChangesHello @pattonwebz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes the foundational REST API endpoint for a new sidebar feature. It introduces a centralized mechanism to fetch all relevant sidebar data for a post, streamlining data retrieval for future UI components. This is an initial integration step, preparing the groundwork for further development and testing of the sidebar functionality. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new REST API endpoint /sidebar-data to fetch data for a sidebar component. The implementation is currently a stub, which is appropriate for an integration branch.
My review has identified a few areas for improvement:
- The new endpoint is missing unit tests. It's important to add tests for the permission logic and basic functionality.
- The code follows an existing pattern of using multiple
add_actioncalls forrest_api_init, which is inefficient. I've recommended refactoring this to improve performance and maintainability, following WordPress best practices. - A minor simplification for the
validate_callbackis suggested to align with common WordPress coding practices. - A placeholder in the PHPDoc for the version number needs to be updated before release.
Overall, the changes are a good start for the new feature, but addressing the feedback, especially adding tests, will be crucial for ensuring quality.
| add_action( | ||
| 'rest_api_init', | ||
| function () use ( $ns, $version ) { | ||
| register_rest_route( | ||
| $ns . $version, | ||
| '/sidebar-data/(?P<id>\d+)', | ||
| [ | ||
| 'methods' => 'GET', | ||
| 'callback' => [ $this, 'get_sidebar_data' ], | ||
| 'args' => [ | ||
| 'id' => [ | ||
| 'required' => true, | ||
| 'validate_callback' => function ( $param ) { | ||
| return is_numeric( $param ); | ||
| }, | ||
| 'sanitize_callback' => 'absint', | ||
| ], | ||
| ], | ||
| 'permission_callback' => function ( $request ) { | ||
| $post_id = (int) $request['id']; | ||
| return current_user_can( 'edit_post', $post_id ); | ||
| }, | ||
| ] | ||
| ); | ||
| } | ||
| ); |
There was a problem hiding this comment.
While this follows the existing pattern in init_rest_routes, adding a separate add_action for each REST route is inefficient and not in line with WordPress best practices. It's better to register all routes within a single callback for the rest_api_init action. Consider refactoring init_rest_routes to use a single add_action('rest_api_init', ...) block to improve performance and code readability.
There was a problem hiding this comment.
Pull request overview
This PR adds a new REST API endpoint for retrieving sidebar metabox data in a single call. The endpoint follows existing patterns in the REST API class with proper permission checking and parameter validation. However, the implementation is currently incomplete as it only returns a stub response.
Changes:
- Added
/sidebar-data/(?P<id>\d+)GET endpoint with post-specific permission validation - Created
get_sidebar_data()method with placeholder implementation returning only post_id
| public function get_sidebar_data( \WP_REST_Request $request ) { | ||
| return new \WP_REST_Response( | ||
| [ | ||
| 'success' => true, | ||
| 'post_id' => (int) $request['id'], | ||
| ], | ||
| 200 | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new get_sidebar_data() endpoint lacks test coverage. Based on the existing REST API test file (RestApiEndpointsTest.php), tests should be added to verify: 1) permission checks ensure only users who can edit the post can access its sidebar data, 2) the endpoint returns expected data structure, and 3) proper HTTP status codes are returned for different scenarios.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
pattonwebz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
includes/classes/class-rest-api.phptests/phpunit/includes/classes/RestApiSidebarDataTest.php
🧰 Additional context used
📓 Path-based instructions (4)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Follow WordPress Coding Standards (WPCS) in all PHP files
Class names use CamelCase (ClassNameConvention) for new classes
Use edac_ prefix for all custom action/filter hook names
Ensure PHP 7.4+ compatibility
Use type hints where appropriate (parameters, return types, properties)
Sanitize inputs, validate data, and escape outputs following WordPress security best practices; use nonces for forms/AJAX
Use the WordPress database API ($wpdb) for all database operations
Prefix functions and classes in the global namespace with edac_
Use WordPress transients for caching temporary data where appropriate
All user-facing text in PHP must be translatable using the accessibility-checker text domain
Use PHPDoc for all public classes, methods, and properties
Document all custom hooks (actions/filters) with docblocks including parameters and types
Files:
includes/classes/class-rest-api.phptests/phpunit/includes/classes/RestApiSidebarDataTest.php
includes/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place core (non-admin) functionality in the /includes directory
Files:
includes/classes/class-rest-api.php
**/class-*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/class-*.php: Legacy PHP class files must use WordPress style naming class-class-name.php
Legacy class names use WordPress underscore style (Class_Name_Convention)
Files:
includes/classes/class-rest-api.php
tests/phpunit/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place and write PHPUnit tests under /tests/phpunit
Files:
tests/phpunit/includes/classes/RestApiSidebarDataTest.php
🧬 Code graph analysis (2)
includes/classes/class-rest-api.php (3)
includes/helper-functions.php (3)
edac_get_valid_table_name(235-258)edac_remove_element_with_value(105-112)edac_filter_by_value(122-137)accessibility-checker.php (1)
edac_register_rules(120-145)admin/class-ajax.php (1)
simplified_summary(788-818)
tests/phpunit/includes/classes/RestApiSidebarDataTest.php (2)
includes/helper-functions.php (1)
edac_get_valid_table_name(235-258)accessibility-checker.php (1)
edac_register_rules(120-145)
⏰ 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). (6)
- GitHub Check: Integration Test: PHP 8.1 | WP latest
- GitHub Check: Integration Test: PHP 8.1 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.2 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.1 | WP latest
- GitHub Check: Integration Test: PHP 8.1 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.2 | WP latest (+ ms)
🔇 Additional comments (13)
includes/classes/class-rest-api.php (6)
209-236: Endpoint registration follows existing patterns and has corresponding test coverage.The new
/sidebar-data/{id}endpoint registration is consistent with other endpoints in this file. The permission callback correctly validatesedit_postcapability for the specific post. Test coverage has been added inRestApiSidebarDataTest.php.
709-736: Clean implementation aggregating sidebar data.The method properly aggregates summary, details, and readability data with appropriate error handling. The try-catch returns a structured error response on failure.
749-766: LGTM!The method correctly handles missing or invalid meta by returning sensible defaults. The defensive check for both existence and array type is appropriate.
778-814: LGTM!The method properly validates the table name, handles the case where no rules exist, and leverages existing helper functions for filtering. The ANWW integration check is consistent with how the plugin handles this elsewhere.
870-920: LGTM!The readability data retrieval is well-implemented with proper defensive checks for the TextStatistics library, consistent use of WordPress filters for content processing, and clear documentation of the grade threshold logic.
832-844: The use of the%iplaceholder is fully compatible. The plugin requires at least WordPress 6.6, which exceeds the WordPress 6.2 requirement for the%iplaceholder in$wpdb->prepare(). No action needed.tests/phpunit/includes/classes/RestApiSidebarDataTest.php (7)
51-67: LGTM!The static setup properly initializes shared fixtures (admin user, test post) and ensures the database table exists for tests. Using
wpSetUpBeforeClassis the correct approach for expensive setup operations.
72-98: LGTM!The setUp and tearDown methods properly initialize the REST server, manage user context, and clean up database entries and filters after each test. This ensures good test isolation.
103-117: LGTM!Test properly verifies the default behavior when post meta is missing.
122-138: LGTM!Test correctly verifies that stored post meta is returned by the summary data helper.
143-194: LGTM!The test properly sets up database state and verifies the details data structure. The conditional check for
$warning_ruleon line 190 is appropriately defensive since the registry may not always have a warning rule available.
199-243: LGTM!The test correctly verifies that issues marked as ignored are excluded from error counts and that the corresponding rule is marked as passed.
281-326: LGTM!The helper methods are well-implemented. The reflection helper follows standard patterns, and
get_sample_rules()appropriately uses an assertion to ensure test prerequisites are met.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| public function test_get_sidebar_data_endpoint_returns_payload() { | ||
| $this->mock_rules( | ||
| [ | ||
| [ | ||
| 'slug' => 'test_error', | ||
| 'rule_type' => 'error', | ||
| ], | ||
| ] | ||
| ); | ||
|
|
||
| $request = new WP_REST_Request( 'GET', '/accessibility-checker/v1/sidebar-data/' . self::$post_id ); | ||
| $request->set_param( 'id', self::$post_id ); | ||
| $response = $this->server->dispatch( $request ); | ||
|
|
||
| $this->assertSame( 200, $response->get_status() ); | ||
| $data = $response->get_data(); | ||
|
|
||
| $this->assertTrue( $data['success'] ); | ||
| $this->assertArrayHasKey( 'data', $data ); | ||
| $this->assertArrayHasKey( 'summary', $data['data'] ); | ||
| $this->assertArrayHasKey( 'details', $data['data'] ); | ||
| $this->assertArrayHasKey( 'readability', $data['data'] ); | ||
| $this->assertSame( self::$post_id, $data['data']['post_id'] ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A 15 'function edac_register_rules' --type=phpRepository: equalizedigital/accessibility-checker
Length of output: 1057
🏁 Script executed:
rg -n -B 3 -A 15 'function mock_rules' --type=phpRepository: equalizedigital/accessibility-checker
Length of output: 1803
🏁 Script executed:
rg -n -B 3 -A 10 'function get_sample_rules' --type=phpRepository: equalizedigital/accessibility-checker
Length of output: 1366
🏁 Script executed:
rg -n -B 5 -A 20 'function setUp|function tearDown|static \$rules|wp_cache_delete|do_action.*reset' tests/phpunit/includes/classes/RestApiSidebarDataTest.phpRepository: equalizedigital/accessibility-checker
Length of output: 1152
🏁 Script executed:
# Check if there's any mechanism to reset the static variable
rg -n 'remove_filter.*edac_filter_register_rules|reset|tearDown' tests/phpunit/includes/classes/RestApiSidebarDataTest.phpRepository: equalizedigital/accessibility-checker
Length of output: 221
🏁 Script executed:
# Look at the full filter application code in edac_register_rules
rg -n -A 25 'function edac_register_rules' accessibility-checker.phpRepository: equalizedigital/accessibility-checker
Length of output: 898
Static variable in edac_register_rules() bypasses the mock_rules() filter on subsequent test calls.
The edac_register_rules() function caches rules in a static variable and returns immediately if the cache is populated (lines 124-125 of accessibility-checker.php), preventing the edac_filter_register_rules filter from being applied on subsequent calls.
While tearDown() removes the mock filter added by mock_rules(), it does not reset the static variable. This means if a prior test populates the cache via get_sample_rules() or similar, the next test's mock_rules() setup will be ignored, and the test will use the previously cached full rule set instead of the mocked single rule.
Consider resetting the static variable between tests, such as by adding a mechanism to clear the cache in tearDown() or by restructuring the function to allow filter-based control without caching constraints.
There was a problem hiding this comment.
This does seem to be an issue, but I don't see a good way to solve it. I like the static cache for rules in this method and don't want to override that as the default behaviour. Do you have any suggestions?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for existing test-related constants or utilities
rg -n 'defined.*PHPUNIT|WP_TESTS_|function.*reset.*cache|wp_cache_flush' --type=php -g '!vendor/**' -g '!node_modules/**' | head -20
# Look at the full edac_register_rules function
cat accessibility-checker.php | sed -n '115,150p'
# Check if there are other functions with similar static cache patterns
rg -n 'static \$.*=.*array\(\)|static \$.*= \[\]' --type=php -A 5 -g '!vendor/**' -g '!node_modules/**' | head -40
Length of output: 2233
@pattonwebz, here are several approaches that preserve the static cache in production while enabling test flexibility:
Option 1: Add a test-aware reset parameter (Recommended)
Add an optional parameter to edac_register_rules() that forces a cache reset when needed:
function edac_register_rules( $force_reload = false ) {
static $default_rules = null;
if ( ! is_null( $default_rules ) && ! $force_reload ) {
return $default_rules;
}
// ... rest of function
}Then in your test's tearDown():
protected function tearDown(): void {
// Reset the static cache for next test
edac_register_rules( true );
// ... rest of tearDown
}Option 2: Check for test environment constant
Following the WP_TESTS_DOMAIN pattern already in your codebase:
function edac_register_rules() {
static $default_rules = null;
// Skip cache in test environment
if ( ! is_null( $default_rules ) && ! defined( 'WP_TESTS_DOMAIN' ) ) {
return $default_rules;
}
// ... rest of function
}This automatically disables caching during tests without any tearDown changes needed.
Option 3: Use reflection in tearDown()
Keep production code unchanged and reset the static using reflection:
protected function tearDown(): void {
// Reset static cache using reflection
$function = new ReflectionFunction( 'edac_register_rules' );
$function->invoke(); // Call once to initialize
$property = new ReflectionProperty( $function, 'static' );
$property->setAccessible( true );
$property->setValue( null, [ 'default_rules' => null ] );
// ... rest of tearDown
}Note: This approach is more complex and may be fragile across PHP versions.
Option 4: Add a dedicated reset function
Create a test utility function in a helper file:
// In tests/phpunit/TestHelpers/test-helpers.php or similar
function edac_test_reset_rules_cache() {
// Force reload by calling with specific logic
add_filter( 'edac_filter_register_rules', '__return_empty_array', 999 );
edac_register_rules();
remove_filter( 'edac_filter_register_rules', '__return_empty_array', 999 );
}My recommendation: Go with Option 1 (reset parameter) as it's explicit, simple, and doesn't impact production performance. The $force_reload parameter makes the intent clear and gives you full control in tests.
There was a problem hiding this comment.
@coderabbitai do you have a suggested method to solve this?
There was a problem hiding this comment.
@pattonwebz, I provided 4 options in my previous reply above, with Option 1 as my recommendation: adding an optional $force_reload parameter to edac_register_rules().
Here's a quick summary of that approach:
Modify edac_register_rules() in accessibility-checker.php:
function edac_register_rules( $force_reload = false ) {
static $default_rules = null;
if ( ! is_null( $default_rules ) && ! $force_reload ) {
return $default_rules;
}
// ... rest of the existing function code
}Add to your test's tearDown() method:
protected function tearDown(): void {
// Reset the static cache for next test
edac_register_rules( true );
// ... rest of existing tearDown code
}This preserves the static cache performance in production while giving tests full control to reset when needed. The parameter defaults to false, so existing calls remain unchanged.
Would you like me to help with the implementation details for any of the other options (environment constant check, reflection, or dedicated reset function)?
|
@pattonwebz I've opened a new pull request, #1334, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@admin/class-enqueue-admin.php`:
- Around line 193-202: The localized data for the 'edac-sidebar' script
(wp_localize_script with handle 'edac-sidebar' and object 'edac_sidebar_app') is
missing the REST nonce; add a 'restNonce' entry to that array using the same
nonce generation used elsewhere (e.g., restNonce => wp_create_nonce('wp_rest'))
so the sidebar's authenticated REST calls (like to the 'sidebar-data' endpoint)
can be validated; update the array alongside 'gutenbergEnabled', 'postID', and
'edacApiUrl' in the wp_localize_script call.
🧹 Nitpick comments (3)
.eslintrc (1)
14-29: LGTM with a minor suggestion.The ESLint overrides are correctly scoped to the sidebar directory and enable JSX support. The
react/jsx-uses-varsrule appropriately prevents false positives for JSX component usage.Consider adding
react/jsx-uses-reactto the rules if you encounter lint errors aboutReactbeing unused, though with the automatic JSX runtime configured in.babelrc, this may not be necessary.package.json (1)
73-75: Consider maintaining alphabetical order for devDependencies.The
@wordpress/i18ndependency is placed at the end of the list, breaking the alphabetical ordering convention. While functionally correct, moving it to its proper position (after@babel/preset-react) would improve maintainability.📦 Suggested ordering
"@babel/preset-react": "^7.22.3", + "@wordpress/i18n": "^6.10.0", "@floating-ui/dom": "^1.2.9", ... - "webpack-cli": "^5.1.1", - "@wordpress/i18n": "^6.10.0" + "webpack-cli": "^5.1.1"admin/class-enqueue-admin.php (1)
199-199: Handle potentialfalsereturn fromget_the_ID().
get_the_ID()can returnfalseif called outside the loop or when no post context exists. While the page guards above should ensure a post context, defensive handling would be more robust, consistent with the pattern used inmaybe_enqueue_admin_and_editor_app_scripts()at line 78.🔧 Proposed fix
+ global $post; + $post_id = is_object( $post ) ? $post->ID : null; + // Localize script with necessary data. wp_localize_script( 'edac-sidebar', 'edac_sidebar_app', [ 'gutenbergEnabled' => true, - 'postID' => get_the_ID(), + 'postID' => $post_id, 'edacApiUrl' => esc_url_raw( rest_url() . 'accessibility-checker/v1' ), + 'restNonce' => wp_create_nonce( 'wp_rest' ), ] );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.babelrc.eslintrcadmin/class-enqueue-admin.phppackage.jsonsrc/sidebar/index.jssrc/sidebar/sidebar.csswebpack.config.js
✅ Files skipped from review due to trivial changes (1)
- src/sidebar/sidebar.css
🧰 Additional context used
📓 Path-based instructions (5)
**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All user-facing JavaScript strings must use wp.i18n for translation
Files:
webpack.config.jssrc/sidebar/index.js
src/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Store source frontend assets (JS/CSS) in /src; do not commit edits directly to built files
Files:
src/sidebar/index.js
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Follow WordPress Coding Standards (WPCS) in all PHP files
Class names use CamelCase (ClassNameConvention) for new classes
Use edac_ prefix for all custom action/filter hook names
Ensure PHP 7.4+ compatibility
Use type hints where appropriate (parameters, return types, properties)
Sanitize inputs, validate data, and escape outputs following WordPress security best practices; use nonces for forms/AJAX
Use the WordPress database API ($wpdb) for all database operations
Prefix functions and classes in the global namespace with edac_
Use WordPress transients for caching temporary data where appropriate
All user-facing text in PHP must be translatable using the accessibility-checker text domain
Use PHPDoc for all public classes, methods, and properties
Document all custom hooks (actions/filters) with docblocks including parameters and types
Files:
admin/class-enqueue-admin.php
admin/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place admin classes and admin-only PHP code in the /admin directory
Files:
admin/class-enqueue-admin.php
**/class-*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/class-*.php: Legacy PHP class files must use WordPress style naming class-class-name.php
Legacy class names use WordPress underscore style (Class_Name_Convention)
Files:
admin/class-enqueue-admin.php
🧠 Learnings (5)
📚 Learning: 2025-08-29T11:15:35.151Z
Learnt from: CR
Repo: equalizedigital/accessibility-checker PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-29T11:15:35.151Z
Learning: Applies to src/**/* : Store source frontend assets (JS/CSS) in /src; do not commit edits directly to built files
Applied to files:
webpack.config.js
📚 Learning: 2025-08-29T11:15:35.151Z
Learnt from: CR
Repo: equalizedigital/accessibility-checker PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-29T11:15:35.151Z
Learning: Applies to **/*.js : All user-facing JavaScript strings must use wp.i18n for translation
Applied to files:
webpack.config.js
📚 Learning: 2025-08-29T11:15:35.151Z
Learnt from: CR
Repo: equalizedigital/accessibility-checker PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-29T11:15:35.151Z
Learning: Applies to accessibility-checker.php : Implement proper plugin activation and deactivation hooks
Applied to files:
src/sidebar/index.js
📚 Learning: 2025-08-26T11:25:48.236Z
Learnt from: pattonwebz
Repo: equalizedigital/accessibility-checker PR: 1208
File: includes/options-page.php:401-410
Timestamp: 2025-08-26T11:25:48.236Z
Learning: In the Accessibility Checker plugin, the edac_is_pro() function uses a robust detection pattern by checking both EDACP_VERSION (which comes from the pro plugin) and EDAC_KEY_VALID existence before looking at its value. This prevents PHP notices and ensures reliable pro feature detection.
Applied to files:
src/sidebar/index.js
📚 Learning: 2025-08-29T11:15:35.151Z
Learnt from: CR
Repo: equalizedigital/accessibility-checker PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-29T11:15:35.151Z
Learning: Applies to tests/jest/**/*.js : Place and write Jest tests under /tests/jest
Applied to files:
.eslintrc
🧬 Code graph analysis (1)
admin/class-enqueue-admin.php (1)
admin/class-settings.php (2)
Settings(13-98)get_scannable_post_types(42-71)
🪛 PHPStan (2.1.33)
admin/class-enqueue-admin.php
176-176: Constant EDAC_PLUGIN_FILE not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(constant.notFound)
191-191: Constant EDAC_PLUGIN_FILE not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(constant.notFound)
207-207: Constant EDAC_PLUGIN_FILE not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(constant.notFound)
⏰ 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). (2)
- GitHub Check: Integration Test: PHP 8.1 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.2 | WP latest (+ ms)
🔇 Additional comments (9)
.babelrc (1)
1-11: LGTM!The Babel configuration correctly enables the automatic JSX runtime, which is the modern approach for React 17+ and eliminates the need to manually import React in every JSX file.
webpack.config.js (2)
20-23: LGTM!The new sidebar entry point correctly bundles both the JavaScript and CSS assets, following the established pattern used by other entries in this configuration.
109-115: LGTM!The externals configuration correctly maps WordPress packages to their global counterparts, ensuring the sidebar bundle leverages WordPress core scripts rather than duplicating them. This aligns with the script dependencies declared in
admin/class-enqueue-admin.php.admin/class-enqueue-admin.php (2)
31-31: LGTM!Appropriate placement for the sidebar script enqueue call within the main enqueue flow.
153-172: Well-structured guards for sidebar script loading.The page and post type validation logic is clean and follows the established patterns in this class. Good use of early returns to avoid unnecessary processing.
package.json (1)
52-52: Confirm the lint configuration is compatible with@wordpress/scriptsv31.The upgrade from
^26.5.0to^31.0.0involves multiple major version increments with dependency and rule changes in@wordpress/eslint-pluginand ESLint. While the code is already deployed at v31, verify thatnpm run lint:jsproduces no new errors and that the GitHub Actions lint workflow is passing without unexpected rule violations. If linting issues emerge after this change, review the .eslintrc and ESLint plugin compatibility.src/sidebar/index.js (3)
5-7: LGTM!The imports are appropriate for a Gutenberg sidebar component, using standard WordPress packages.
12-24: LGTM!The component follows Gutenberg patterns correctly, and all user-facing strings use the
__()translation function with the appropriate text domain, as per coding guidelines.
26-31: LGTM!The conditional registration is a good defensive pattern that ensures the sidebar only registers when the required global data is available and Gutenberg is enabled.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Localize script with necessary data. | ||
| wp_localize_script( | ||
| 'edac-sidebar', | ||
| 'edac_sidebar_app', | ||
| [ | ||
| 'gutenbergEnabled' => true, | ||
| 'postID' => get_the_ID(), | ||
| 'edacApiUrl' => esc_url_raw( rest_url() . 'accessibility-checker/v1' ), | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Missing restNonce for authenticated REST API calls.
The localized data for the sidebar script does not include restNonce, which is required for authenticated REST API requests. Other scripts in this file (e.g., edac_script_vars at line 89, edac_editor_app at line 145) include this nonce. Without it, the sidebar's API calls to the sidebar-data endpoint may fail authentication.
🔧 Proposed fix
wp_localize_script(
'edac-sidebar',
'edac_sidebar_app',
[
'gutenbergEnabled' => true,
'postID' => get_the_ID(),
'edacApiUrl' => esc_url_raw( rest_url() . 'accessibility-checker/v1' ),
+ 'restNonce' => wp_create_nonce( 'wp_rest' ),
]
);🤖 Prompt for AI Agents
In `@admin/class-enqueue-admin.php` around lines 193 - 202, The localized data for
the 'edac-sidebar' script (wp_localize_script with handle 'edac-sidebar' and
object 'edac_sidebar_app') is missing the REST nonce; add a 'restNonce' entry to
that array using the same nonce generation used elsewhere (e.g., restNonce =>
wp_create_nonce('wp_rest')) so the sidebar's authenticated REST calls (like to
the 'sidebar-data' endpoint) can be validated; update the array alongside
'gutenbergEnabled', 'postID', and 'edacApiUrl' in the wp_localize_script call.
There was a problem hiding this comment.
I will be tackling this later when I add buttons for actions like rescan and clear issues.
There was a problem hiding this comment.
@pattonwebz, sounds good! Since this will be addressed in a future change when you add the action buttons, would you like me to create an issue to track adding the restNonce to the sidebar's localized data? That way it won't be forgotten when implementing the rescan and clear issues functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/sidebar/components/QuickAccessPanel.js`:
- Line 18: The component uses useDispatch('core/edit-post') and
PluginDocumentSettingPanel which only exist in the post editor, but registration
is gated only by window.edac_sidebar_app.gutenbergEnabled; update
QuickAccessPanel to explicitly guard for the post editor or document the
requirement: modify the QuickAccessPanel component to check the editor context
(e.g., verify wp.data.select('core/edit-post') or a safe flag) before calling
useDispatch/openGeneralSidebar and render a harmless fallback (null or message)
in FSE/site-editor contexts, or add a clear comment noting that
window.edac_sidebar_app.gutenbergEnabled is guaranteed to be set only in post
editor PHP registration; reference QuickAccessPanel, openGeneralSidebar,
PluginDocumentSettingPanel and window.edac_sidebar_app.gutenbergEnabled when
making the change.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/sidebar/components/QuickAccessPanel.jssrc/sidebar/index.jssrc/sidebar/sass/components/quick-access-panel.scss
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sidebar/index.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All user-facing JavaScript strings must use wp.i18n for translation
Files:
src/sidebar/components/QuickAccessPanel.js
src/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Store source frontend assets (JS/CSS) in /src; do not commit edits directly to built files
Files:
src/sidebar/components/QuickAccessPanel.jssrc/sidebar/sass/components/quick-access-panel.scss
🧠 Learnings (1)
📓 Common learnings
Learnt from: SteveJonesDev
Repo: equalizedigital/accessibility-checker PR: 1088
File: includes/classes/Rules/Rule/VideoPresentRule.php:41-42
Timestamp: 2025-07-18T01:18:27.812Z
Learning: In the Accessibility Checker plugin, the 'wcag' field can use custom values like '0.3' to represent best practice rules and manual testing requirements that don't map directly to specific WCAG success criteria but are still important for accessibility compliance.
Learnt from: CR
Repo: equalizedigital/accessibility-checker PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-29T11:15:35.151Z
Learning: Applies to accessibility-checker.php : Implement proper plugin activation and deactivation hooks
Learnt from: SteveJonesDev
Repo: equalizedigital/accessibility-checker PR: 1088
File: includes/classes/Rules/Rule/EmptyParagraphTagRule.php:50-50
Timestamp: 2025-07-18T01:29:35.548Z
Learning: In the Accessibility Checker plugin, the 'wcag' field can use custom values like '0.1' to represent best practice rules and manual testing requirements that don't map directly to specific WCAG success criteria but are still important for accessibility compliance.
⏰ 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). (1)
- GitHub Check: Integration Test: PHP 8.1 | WP latest (+ ms)
🔇 Additional comments (2)
src/sidebar/components/QuickAccessPanel.js (1)
1-46: LGTM! Well-structured component with proper i18n.The component correctly uses
__()from@wordpress/i18nfor all user-facing strings, follows WordPress/Gutenberg patterns, and properly memoizes the click handler with the correct dependency array.src/sidebar/sass/components/quick-access-panel.scss (1)
1-14: LGTM! Clean BEM styling that aligns with the component.The SCSS follows BEM conventions and correctly corresponds to the class names used in
QuickAccessPanel.js. The styles are minimal and rely appropriately on WordPress'sPanelRowcomponent providing the base flexbox context.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/sidebar/components/SidebarContent.js`:
- Around line 26-32: The imported store file (STORE_NAME from
./store/accessibility-checker-store) is missing; create that module exporting
STORE_NAME and a store implementation that provides a getError() selector used
by SidebarContent.js and action handlers to set/clear errors (e.g., setError,
clearError) so the hook can call getError() successfully; ensure the store
registers under the exported STORE_NAME and exposes the selector getError() (and
corresponding actions) matching the usage in SidebarContent.js.
In `@src/sidebar/hooks/useAccessibilityCheckerData.js`:
- Around line 16-36: The hook useAccessibilityCheckerData is defined without
parameters but QuickAccessPanel.js calls it with postId; update
useAccessibilityCheckerData to accept a postId parameter and pass that postId
into the store selectors and refetch call: call
select(STORE_NAME).getData(postId) (and likewise pass postId to
isLoading/getError/isRefreshing if those selectors accept it), and wrap the
dispatched refetchData so the returned refetch function invokes
refetchData(postId); ensure the symbol names referenced are
useAccessibilityCheckerData, getData, isLoading, getError, isRefreshing,
refetchData and the caller QuickAccessPanel.js is left unchanged.
🧹 Nitpick comments (3)
src/sidebar/sass/components/sidebar-content.scss (1)
1-56: LGTM!Well-structured SCSS that follows the component hierarchy. The BEM-style modifiers for error/warning states are clean.
Consider extracting repeated color values (e.g.,
#50575efor empty states,#d63638for errors) into SCSS variables for easier theming and consistency across the sidebar styles.src/sidebar/index.js (1)
70-72: Consider simplifying by removing the trivial wrapper.
QuickAccessPanelWrapperjust returns<QuickAccessPanel />without any additional logic. You could passQuickAccessPaneldirectly toregisterPlugin.♻️ Suggested simplification
-/** - * Quick access panel wrapper component - */ -function QuickAccessPanelWrapper() { - return <QuickAccessPanel />; -} - // Register the combined component if ( window.edac_sidebar_app && window.edac_sidebar_app.gutenbergEnabled ) { registerPlugin( 'accessibility-checker', { render: AccessibilityCheckerSidebar, } ); registerPlugin( 'accessibility-checker-quick-access', { - render: QuickAccessPanelWrapper, + render: QuickAccessPanel, } ); }src/sidebar/components/QuickAccessPanel.js (1)
68-100: Consider handling the warnings-only case more gracefully.When
errorCount = 0andwarningCount > 0, the message would read "You have 0 problems to address and X issues that need review." Consider restructuring to show only the warnings message when there are no errors:♻️ Suggested improvement for warnings-only case
} else if ( errorCount > 0 || warningCount > 0 ) { - // Build translatable summary message - const errorText = _n( - 'problem to address', - 'problems to address', - errorCount, - 'accessibility-checker', - ); - let summaryText; - if ( warningCount > 0 ) { + if ( errorCount > 0 && warningCount > 0 ) { + const errorText = _n( + 'problem to address', + 'problems to address', + errorCount, + 'accessibility-checker', + ); const warningText = _n( 'issue that needs review', 'issues that need review', warningCount, 'accessibility-checker', ); summaryText = sprintf( // translators: %1$d = number of errors, %2$s = error text, %3$d = number of warnings, %4$s = warning text __( 'You have %1$d %2$s and %3$d %4$s.', 'accessibility-checker' ), errorCount, errorText, warningCount, warningText, ); + } else if ( errorCount > 0 ) { + const errorText = _n( + 'problem to address', + 'problems to address', + errorCount, + 'accessibility-checker', + ); + summaryText = sprintf( + // translators: %1$d = number of errors, %2$s = error text + __( 'You have %1$d %2$s.', 'accessibility-checker' ), + errorCount, + errorText, + ); } else { + const warningText = _n( + 'issue that needs review', + 'issues that need review', + warningCount, + 'accessibility-checker', + ); summaryText = sprintf( - // translators: %1$d = number of errors, %2$s = error text - __( 'You have %1$d %2$s.', 'accessibility-checker' ), - errorCount, - errorText, + // translators: %1$d = number of warnings, %2$s = warning text + __( 'You have %1$d %2$s.', 'accessibility-checker' ), + warningCount, + warningText, ); }
| export const useAccessibilityCheckerData = () => { | ||
| const { data, loading, error, refreshing } = useSelect( | ||
| ( select ) => ( { | ||
| data: select( STORE_NAME ).getData(), | ||
| loading: select( STORE_NAME ).isLoading(), | ||
| error: select( STORE_NAME ).getError(), | ||
| refreshing: select( STORE_NAME ).isRefreshing(), | ||
| } ), | ||
| [], | ||
| ); | ||
|
|
||
| const { refetchData } = useDispatch( STORE_NAME ); | ||
|
|
||
| return { | ||
| data, | ||
| loading, | ||
| error, | ||
| refreshing, | ||
| refetch: refetchData, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "useAccessibilityCheckerData" --type js -C3Repository: equalizedigital/accessibility-checker
Length of output: 3075
Fix hook signature mismatch in useAccessibilityCheckerData.
The hook is defined without parameters but is called with a postId argument in QuickAccessPanel.js (line 28):
const { data, loading, refreshing } = useAccessibilityCheckerData( postId );The hook definition does not accept or use this parameter. Either update the hook to accept and pass postId to the store's getData() method, or remove the argument from the caller in QuickAccessPanel.js.
🤖 Prompt for AI Agents
In `@src/sidebar/hooks/useAccessibilityCheckerData.js` around lines 16 - 36, The
hook useAccessibilityCheckerData is defined without parameters but
QuickAccessPanel.js calls it with postId; update useAccessibilityCheckerData to
accept a postId parameter and pass that postId into the store selectors and
refetch call: call select(STORE_NAME).getData(postId) (and likewise pass postId
to isLoading/getError/isRefreshing if those selectors accept it), and wrap the
dispatched refetchData so the returned refetch function invokes
refetchData(postId); ensure the symbol names referenced are
useAccessibilityCheckerData, getData, isLoading, getError, isRefreshing,
refetchData and the caller QuickAccessPanel.js is left unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/sidebar/store/accessibility-checker-store.js`:
- Around line 136-143: The reducer case 'UPDATE_READABILITY_DATA' can throw when
state.data is null because spreading null is invalid; update that case to guard
by using a fallback object (e.g., replace ...state.data with ...(state.data ||
{}) or create/initialize data if missing) so the returned state sets data: {
...(state.data || {}), readability: action.readabilityData } (or return state
unchanged if you prefer), ensuring you reference the 'UPDATE_READABILITY_DATA'
case and the 'state.data' field when making the change.
♻️ Duplicate comments (1)
src/sidebar/hooks/useAccessibilityCheckerData.js (1)
16-37: Verify hook usage consistency across callers.The hook signature takes no parameters, which is correct if data fetching is centralized at the root component (as noted in the docblock). However, a previous review flagged that
QuickAccessPanel.jswas calling this hook with apostIdargument that gets silently ignored.Please verify that all callers have been updated to call the hook without arguments, or if
postIdis needed, update the hook signature and selectors accordingly.#!/bin/bash # Description: Find all usages of useAccessibilityCheckerData to verify call consistency # Search for all calls to useAccessibilityCheckerData and show context rg -n "useAccessibilityCheckerData\s*\(" --type js -C2
🧹 Nitpick comments (6)
src/sidebar/components/Icon.js (1)
102-149: Consider addingrole="img"whenariaLabelis provided.This helps some assistive tech reliably announce the label for non-semantic elements.
♻️ Suggested tweak
- const ariaProps = { - 'aria-hidden': resolvedAriaHidden, - }; - - if ( ariaLabel ) { - ariaProps[ 'aria-label' ] = ariaLabel; - } + const ariaProps = { + 'aria-hidden': resolvedAriaHidden, + ...( ariaLabel ? { 'aria-label': ariaLabel, role: 'img' } : {} ), + };src/sidebar/components/ReadabilityAnalysis.js (3)
123-131: Minor i18n concern with ordinal suffix.The
%dth gradeformat at line 128 assumes English ordinal suffix pattern. However, sincepostGradeReadable(line 124-126) is the preferred path and likely contains properly formatted text, this fallback may rarely be used. Consider ifpostGradeReadableis always available.
137-156: Simplify redundant conditional check.Line 144's condition
readingLevelStatus !== 'below'is always true at that point since line 141-143 already returns early when it equals'below'. This can be simplified.♻️ Suggested simplification
const getPanelIcon = () => { if ( ! hasContent || postGrade === 0 || postGrade === undefined || postGrade === null ) { return 'warning'; } if ( readingLevelStatus === 'below' ) { return 'check'; } - if ( readingLevelStatus !== 'below' ) { - if ( ! summaryText ) { - return 'warning'; - } - if ( summaryGrade > 0 && ! summaryGradeFailed ) { - return 'check'; - } - if ( summaryGradeFailed ) { - return 'warning'; - } + // readingLevelStatus is 'above' at this point + if ( ! summaryText ) { + return 'warning'; + } + if ( summaryGrade > 0 && ! summaryGradeFailed ) { + return 'check'; } return 'warning'; };
213-214: Fallbackhref="#"may cause accessibility issues.When
settingsUrlis undefined, the fallbackhref="#"creates a link that scrolls to the page top without navigating anywhere. Consider conditionally rendering the link or usingjavascript:void(0)with appropriate ARIA, or better yet, render a<Button variant="link">when no URL is available.This pattern appears on lines 213, 237, 263, 268, and 306.
includes/classes/class-rest-api.php (2)
876-889: Consider using$wpdb->prepare()for SQL safety.While
esc_sql()is applied to values, WordPress coding standards prefer$wpdb->prepare()for parameterized queries. The current approach works but is harder to audit. The$post_idvariable comes from the request and should be explicitly cast here for defense-in-depth.♻️ Suggested improvement using prepare()
- // Build a simple, escaped IN clause. - $safe_table = esc_sql( $table_name ); - $escaped_slugs = array_map( 'esc_sql', $rule_slugs ); - $in_clause = "'" . implode( "','", $escaped_slugs ) . "'"; - - // Direct SQL query (table and values already escaped). - $sql = "SELECT id, postid, object, ruletype, rule, ignre, ignre_user, ignre_date, ignre_comment\n" - . "FROM `{$safe_table}`\n" - . "WHERE postid = {$post_id}\n" - . "AND rule IN ( {$in_clause} )\n" - . "AND siteid = {$siteid}\n" - . 'AND ignre = 0'; + // Build placeholders for prepared statement. + $post_id = (int) $post_id; + $siteid = (int) $siteid; + $safe_table = esc_sql( $table_name ); + $placeholders = implode( ', ', array_fill( 0, count( $rule_slugs ), '%s' ) ); + + // Use prepared statement for the dynamic values. + $sql = $wpdb->prepare( + "SELECT id, postid, object, ruletype, rule, ignre, ignre_user, ignre_date, ignre_comment + FROM `{$safe_table}` + WHERE postid = %d + AND rule IN ( {$placeholders} ) + AND siteid = %d + AND ignre = 0", + array_merge( [ $post_id ], $rule_slugs, [ $siteid ] ) + );
990-1000: Double sanitization is redundant but harmless.Line 992 applies
sanitize_textarea_field()to$request['summary'], but the REST route definition (line 258) already specifies'sanitize_callback' => 'sanitize_textarea_field'. The double sanitization is defensive coding but could be simplified.♻️ Suggested simplification
public function save_simplified_summary( \WP_REST_Request $request ) { $post_id = (int) $request['id']; - $summary = sanitize_textarea_field( wp_unslash( $request['summary'] ) ); + $summary = $request['summary']; // Already sanitized by REST route sanitize_callback // Update the post meta with the simplified summary (matching AJAX behavior). update_post_meta(
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/admin/index.js`:
- Around line 445-453: The dispatched CustomEvent 'edac-ignore-updated'
currently sets postId using parseInt( jQuery('#post_ID').val() ) which can yield
NaN if the element is missing or empty; update the code that constructs the
event detail to guard against NaN by falling back to the known post ID
(edacScriptVars.postID or a numeric default) — e.g., read the value from
jQuery('#post_ID'), attempt Number/parseInt conversion, test isNaN and if true
use edacScriptVars.postID (or 0) before creating the CustomEvent so listeners
always receive a valid numeric postId.
In `@src/sidebar/components/AccessibilityStatus.js`:
- Around line 73-78: The fallback formatting for postGrade in
AccessibilityStatus.js incorrectly uses '%dth' and doesn't handle English
ordinal variations; update the logic in the block that sets readingLevelText
(where postGrade and postGradeReadable are used) to compute a proper ordinal
string when postGradeReadable is missing by adding a small helper (e.g.,
getOrdinalSuffix or formatOrdinal) that returns "1st/2nd/3rd/21st/etc." and use
that helper to produce the final readingLevelText instead of sprintf('%dth',
postGrade).
♻️ Duplicate comments (1)
admin/class-enqueue-admin.php (1)
195-207: Verify the REST nonce key expected by the sidebar JS.The localized object uses
nonce, while other script locals userestNonce. If the sidebar code expectsrestNonce, REST auth will fail. Please confirm the JS usage and align the key name accordingly.#!/bin/bash # Verify which key the sidebar JS expects for the REST nonce. rg -n -C3 "edac_sidebar_app" -g '*.{js,jsx,ts,tsx}' rg -n -C3 "restNonce|nonce" -g '*.{js,jsx,ts,tsx}'🛠️ Optional fix if the JS expects
restNonce- 'nonce' => wp_create_nonce( 'wp_rest' ), + 'restNonce' => wp_create_nonce( 'wp_rest' ),
🧹 Nitpick comments (4)
src/sidebar/hooks/useAccessibilityCheckerData.js (1)
31-36: WraprefetchinuseCallbackto stabilize its identity.The
refetchfunction is included in useEffect dependency arrays by consumers (e.g.,AccessibilityStatus.jsline 35). WithoutuseCallback, a new function is created on every render, potentially causing unnecessary effect re-runs.♻️ Suggested fix
+import { useCallback } from '@wordpress/element'; + // Wrap refetchData to automatically include postId -const refetch = () => { +const refetch = useCallback( () => { if ( postId ) { refetchData( postId ); } -}; +}, [ postId, refetchData ] );src/admin/index.js (1)
73-76: Minor: Remove unused event parameter.The
eventparameter is not used in the callback.♻️ Suggested fix
// Listen for simplified summary save from Gutenberg sidebar -window.addEventListener( 'edac-simplified-summary-saved', function( event ) { +window.addEventListener( 'edac-simplified-summary-saved', function() { refreshSummaryAndReadability(); } );src/sidebar/sass/components/accessibility-status.scss (1)
27-29: Consider using a color variable for hover state.The hardcoded
#f5f5f5could be replaced with a variable from_variables.scssfor consistent theming.src/sidebar/components/AccessibilityStatus.js (1)
22-35: Consider documenting the delay reason or extracting as a constant.The 300ms timeout is a reasonable delay for allowing the ignore save to complete, but a brief comment or named constant would improve maintainability.
♻️ Suggested improvement
+// Delay to allow the ignore save operation to complete on the server +const REFETCH_DELAY_MS = 300; + // Listen for ignore updates from the old metabox and refetch data useEffect( () => { const handleIgnoreUpdated = () => { - // Small delay so the ignore save can complete before we refetch. window.setTimeout( () => { refetch(); - }, 300 ); + }, REFETCH_DELAY_MS ); };
| // Dispatch event to notify sidebar that ignore action was completed | ||
| const event = new CustomEvent( 'edac-ignore-updated', { | ||
| detail: { | ||
| postId: parseInt( jQuery( '#post_ID' ).val() ), | ||
| action: data.action, | ||
| ruleId: data.rule_id, | ||
| }, | ||
| } ); | ||
| window.dispatchEvent( event ); |
There was a problem hiding this comment.
Guard against potential NaN when dispatching event.
If #post_ID element is missing or its value is empty, parseInt() returns NaN, which would propagate to event listeners. Consider using a fallback or the existing postID variable from edacScriptVars.postID.
🔧 Suggested fix
// Dispatch event to notify sidebar that ignore action was completed
const event = new CustomEvent( 'edac-ignore-updated', {
detail: {
- postId: parseInt( jQuery( '#post_ID' ).val() ),
+ postId: edacScriptVars.postID || parseInt( jQuery( '#post_ID' ).val() ) || null,
action: data.action,
ruleId: data.rule_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.
| // Dispatch event to notify sidebar that ignore action was completed | |
| const event = new CustomEvent( 'edac-ignore-updated', { | |
| detail: { | |
| postId: parseInt( jQuery( '#post_ID' ).val() ), | |
| action: data.action, | |
| ruleId: data.rule_id, | |
| }, | |
| } ); | |
| window.dispatchEvent( event ); | |
| // Dispatch event to notify sidebar that ignore action was completed | |
| const event = new CustomEvent( 'edac-ignore-updated', { | |
| detail: { | |
| postId: edacScriptVars.postID || parseInt( jQuery( '#post_ID' ).val() ) || null, | |
| action: data.action, | |
| ruleId: data.rule_id, | |
| }, | |
| } ); | |
| window.dispatchEvent( event ); |
🤖 Prompt for AI Agents
In `@src/admin/index.js` around lines 445 - 453, The dispatched CustomEvent
'edac-ignore-updated' currently sets postId using parseInt(
jQuery('#post_ID').val() ) which can yield NaN if the element is missing or
empty; update the code that constructs the event detail to guard against NaN by
falling back to the known post ID (edacScriptVars.postID or a numeric default) —
e.g., read the value from jQuery('#post_ID'), attempt Number/parseInt
conversion, test isNaN and if true use edacScriptVars.postID (or 0) before
creating the CustomEvent so listeners always receive a valid numeric postId.
| if ( postGrade > 0 ) { | ||
| if ( postGradeReadable ) { | ||
| readingLevelText = postGradeReadable; | ||
| } else { | ||
| readingLevelText = sprintf( __( '%dth', 'accessibility-checker' ), postGrade ); | ||
| } |
There was a problem hiding this comment.
Ordinal suffix handling may be incorrect for some grades.
The fallback '%dth' doesn't handle English ordinal variations (1st, 2nd, 3rd, 21st, 22nd, etc.). Since postGradeReadable is preferred when available, this may be acceptable as a rare fallback, but consider using a proper ordinal formatting function for correctness.
💡 Example ordinal helper
const getOrdinalSuffix = ( n ) => {
const s = [ 'th', 'st', 'nd', 'rd' ];
const v = n % 100;
return n + ( s[ ( v - 20 ) % 10 ] || s[ v ] || s[ 0 ] );
};🤖 Prompt for AI Agents
In `@src/sidebar/components/AccessibilityStatus.js` around lines 73 - 78, The
fallback formatting for postGrade in AccessibilityStatus.js incorrectly uses
'%dth' and doesn't handle English ordinal variations; update the logic in the
block that sets readingLevelText (where postGrade and postGradeReadable are
used) to compute a proper ordinal string when postGradeReadable is missing by
adding a small helper (e.g., getOrdinalSuffix or formatOrdinal) that returns
"1st/2nd/3rd/21st/etc." and use that helper to produce the final
readingLevelText instead of sprintf('%dth', postGrade).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin/class-enqueue-admin.php (1)
66-75:⚠️ Potential issue | 🔴 CriticalFix logic inversion in script enqueue condition.
The condition
'site-editor.php' !== $pagenowevaluates totrueon virtually every admin page (all pages that are NOT site-editor.php). Combined with||, this causes the entire condition to pass for almost all admin pages, loading scripts unnecessarily and defeating the purpose of conditional enqueue. The condition should use===to specifically include the site-editor:🔧 Suggested fix
if ( ( $has_post_types && ( $is_scannable_post || in_array( $page, $enabled_pages, true ) ) ) || - 'site-editor.php' !== $pagenow + 'site-editor.php' === $pagenow ) {
🤖 Fix all issues with AI agents
In `@admin/class-enqueue-admin.php`:
- Around line 195-211: Rename the REST API nonce key in the wp_localize_script
call that defines edac_sidebar_app from 'nonce' to 'restNonce' to match the
project's convention (used elsewhere with edac_script_vars and edac_editor_app);
update the array key in the wp_localize_script payload so the REST nonce is
created with wp_create_nonce('wp_rest') but exposed as 'restNonce', leaving
other keys (like 'ajaxNonce') unchanged and ensuring all consumer code that
reads edac_sidebar_app expects restNonce.
In `@src/sidebar/components/IssueDetailsModal.js`:
- Around line 91-98: The code calls JSON.parse(data.data) in dismissIssue (and
similarly in undismissIssue) which will throw on malformed JSON; wrap the parse
in a try-catch (or validate that data.data is already an object) and on parse
failure throw a new Error that includes the raw response or parse error details
so the exception is handled and debuggable — e.g. in dismissIssue and
undismissIssue, replace the direct JSON.parse(data.data) with a guarded parse:
try to parse, catch the SyntaxError, and throw a descriptive Error containing
the parse error and the original data.data.
In `@src/sidebar/components/RuleAccordion.js`:
- Around line 20-34: getViewOnPageUrl currently calls new URL(viewLink) which
will throw a TypeError for invalid viewLink strings; wrap the URL construction
in a try-catch inside getViewOnPageUrl, return null on any failure, and only
manipulate searchParams and return url.toString() when the URL was successfully
created (preserve behavior for highlightNonce and edac param). Ensure you
reference getViewOnPageUrl and handle the case where viewLink is falsy before
attempting URL creation.
🧹 Nitpick comments (15)
includes/classes/class-rest-api.php (1)
876-889: Prefer$wpdb->prepare()for SQL query construction.While
$post_idand$siteidare integers (sanitized viaabsintandget_current_blog_id()), the recommended WordPress practice is to use$wpdb->prepare()for all dynamic values in SQL queries. This provides defense-in-depth and maintains consistency with WordPress coding standards.🔧 Suggested refactor
- // Build a simple, escaped IN clause. - $safe_table = esc_sql( $table_name ); - $escaped_slugs = array_map( 'esc_sql', $rule_slugs ); - $in_clause = "'" . implode( "','", $escaped_slugs ) . "'"; - - // Direct SQL query (table and values already escaped). - $sql = "SELECT id, postid, object, ruletype, rule, ignre, ignre_user, ignre_date, ignre_comment\n" - . "FROM `{$safe_table}`\n" - . "WHERE postid = {$post_id}\n" - . "AND rule IN ( {$in_clause} )\n" - . "AND siteid = {$siteid}\n" - . 'AND ignre = 0'; - - // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching, WordPress.DB.PreparedSQL.NotPrepared - $all_results = $wpdb->get_results( $sql, ARRAY_A ); + // Build placeholders for IN clause. + $placeholders = implode( ',', array_fill( 0, count( $rule_slugs ), '%s' ) ); + $safe_table = esc_sql( $table_name ); + + // Use prepared statement for all dynamic values. + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching + $all_results = $wpdb->get_results( + $wpdb->prepare( + "SELECT id, postid, object, ruletype, rule, ignre, ignre_user, ignre_date, ignre_comment + FROM `{$safe_table}` + WHERE postid = %d + AND rule IN ( {$placeholders} ) + AND siteid = %d + AND ignre = 0", + array_merge( [ $post_id ], $rule_slugs, [ $siteid ] ) + ), + ARRAY_A + );src/sidebar/sass/components/sidebar-content.scss (2)
70-71: Remove empty rule block.The
.edac-panel-rowselector is empty and serves no purpose. Either add styles or remove it.🧹 Proposed fix
-.edac-panel-row { -} - .edac-panel-section {
49-62: Consider using a variable for the repeated gray color.The color
#50575eis hardcoded in three places (--empty, combined modifiers, and__description). For consistency with the token-based approach used elsewhere (e.g.,$error-red,$warning-orange), consider defining a variable like$neutral-greyin_variables.scss.src/sidebar/sass/components/issue-details-modal.scss (1)
1-144: Consider importing shared variables for color consistency.Unlike other SCSS files in this PR (e.g.,
sidebar-content.scss,accessibility-status.scss), this file doesn't import the shared_variables.scss. Colors like#ddd,#f5f5f5,#0073aa, and#f0f6fcare hardcoded throughout. For theming consistency, consider using@use '../_variables' as *;and replacing hardcoded values with tokens where applicable (e.g.,$outline-grey,$info-blue).src/sidebar/sass/components/accessibility-status.scss (1)
27-29: Minor: Hardcoded hover color.Line 28 uses hardcoded
#f5f5f5while the rest of the file consistently uses token variables. Consider adding a hover background token if one exists, or documenting why this specific value is used.src/sidebar/sass/components/accessibility-analysis-tabs.scss (2)
1-85: Consider importing shared variables for consistency.Similar to
issue-details-modal.scss, this file uses hardcoded colors (#ddd,#0073aa,#f0f6fc, etc.) instead of importing from_variables.scss. While some may be intentional WordPress admin colors, using shared tokens would improve maintainability.
78-80: Remove empty rule block.The
&__tabselector is empty. Either add the intended styles or remove it.🧹 Proposed fix
} - &__tab { - // Additional styles for custom tab class if needed - } - &__count {src/sidebar/components/AccessibilityAnalysisTabs.js (1)
59-68: Potential key collision ifrule.titleis used as fallback.The
ruleIdfallback chain (rule.slug || rule.id || rule.title) usesrule.titleas the last resort. If multiple rules share the same title (unlikely but possible), React will log key warnings and may exhibit unexpected behavior.Consider adding an index-based suffix as a safeguard:
const ruleId = rule.slug || rule.id || `${rule.title}-${index}`;However, if the data contract guarantees unique
slugoridvalues, this is a non-issue.src/sidebar/sass/components/accessibility-analysis.scss (2)
30-34: Consider using SCSS variables for all colors to improve maintainability.The file mixes hardcoded color values (
#666,#ddd,#fff,#0073aa,#1e1e1e,#f9f9f9,#fafafa) with variable-based tokens ($outline-grey,$severity-*). For easier theming and consistency, consider defining variables for common colors like borders, text colors, and backgrounds in_variables.scss.Also applies to: 50-54, 68-73, 80-81, 91-94, 141-141, 152-164, 167-168, 178-180, 211-211, 217-217, 221-221, 242-243, 266-267
148-150: Inconsistent indentation on line 149.Line 149 uses spaces for indentation while the rest of the file uses tabs. This should be corrected for consistency.
🔧 Proposed fix
&__issue-list { list-style: none; margin: 0; padding: 0; border: 1px solid $outline-grey; - border-radius: 8px; + border-radius: 8px; }src/sidebar/components/AccessibilityStatus.js (2)
66-76: Consider a more robust approach for opening the accordion.The current implementation relies on finding elements by class names (
.edac-readability-analysis,.edac-accordion__button,.edac-accordion--closed). If class names change, this will silently fail. Consider using a shared state mechanism or callback prop to coordinate accordion expansion.
22-35: The 300ms delay is a pragmatic workaround but may be fragile.The setTimeout delay to allow the ignore save to complete before refetching could fail if the server is slow. Consider listening for a more specific event that signals completion, or implementing a retry mechanism.
src/sidebar/components/RuleAccordion.js (3)
203-206: Remove debugconsole.logbefore merging.The
console.logstatement is left with an eslint-disable comment. Consider removing it or replacing with a proper logging mechanism.🧹 Proposed fix
- // eslint-disable-next-line no-console - console.log( `Action: ${ action }`, issue ); // TODO: Implement remaining actions (fix)
207-210: EmptyhandleIgnorecallback has no effect.The
handleIgnorefunction is passed toIssueDetailsModalbut does nothing. If no action is needed when an issue is ignored (since the AJAX already completed), consider removing the callback entirely or documenting why it's intentionally empty.
65-74: Consider handling unknown severity values more gracefully.If
severityis not 1-4 or a valid string,getSeverityLabelreturns an empty string, resulting in a CSS classedac-analysis__badge--(with trailing dash). Consider either not rendering the badge for unknown severities or providing a default class.💡 Example fix in SeverityBadge
const SeverityBadge = ( { severity } ) => { const severityLabel = getSeverityLabel( severity ); + if ( ! severityLabel ) { + return null; + } const severityKey = severityLabel.toLowerCase(); return ( <span className={ `edac-analysis__badge edac-analysis__badge--${ severityKey }` }> { severityLabel } </span> ); };
| // Localize script with necessary data. | ||
| wp_localize_script( | ||
| 'edac-sidebar', | ||
| 'edac_sidebar_app', | ||
| [ | ||
| 'gutenbergEnabled' => true, | ||
| 'postID' => get_the_ID(), | ||
| 'highlightNonce' => wp_create_nonce( 'edac_highlight' ), | ||
| 'ajaxNonce' => wp_create_nonce( 'ajax-nonce' ), | ||
| 'ajaxUrl' => admin_url( 'admin-ajax.php' ), | ||
| 'edacApiUrl' => esc_url_raw( rest_url() . 'accessibility-checker/v1' ), | ||
| 'nonce' => wp_create_nonce( 'wp_rest' ), | ||
| 'settingsUrl' => esc_url_raw( admin_url( 'admin.php?page=accessibility_checker_settings' ) ), | ||
| 'readabilityHelpUrl' => esc_url_raw( edac_link_wrapper( 'https://a11ychecker.com/help3265', 'wordpress-general', 'content-analysis-sidebar', false ) ), | ||
| 'manuallyTestHelpUrl' => esc_url_raw( edac_link_wrapper( 'https://a11ychecker.com/help4280', 'wordpress-general', 'content-analysis-sidebar', false ) ), | ||
| ] | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check client-side code for nonce key usage
rg -n "edac_sidebar_app\.(nonce|restNonce)" -t js
# Also check broader patterns in JS/sidebar code
rg -n "\.nonce\b|\.restNonce\b" -t js | head -30Repository: equalizedigital/accessibility-checker
Length of output: 1530
🏁 Script executed:
# Verify the nonce key usage in the PHP file itself at the mentioned lines
sed -n '85,95p; 139,149p; 200,215p' admin/class-enqueue-admin.phpRepository: equalizedigital/accessibility-checker
Length of output: 1681
🏁 Script executed:
# Find all localize_script calls in the file to see naming patterns
rg -A 15 "wp_localize_script" admin/class-enqueue-admin.phpRepository: equalizedigital/accessibility-checker
Length of output: 2192
Nonce key naming inconsistency in edac_sidebar_app localization.
The REST API nonce is named nonce at line 206, while other scripts consistently use restNonce for REST API nonces (e.g., edac_script_vars at line 89, edac_editor_app at line 143). The pattern throughout the codebase reserves 'nonce' for AJAX nonces and 'restNonce' for REST API nonces.
Rename to align with the established convention:
🔧 Suggested fix
wp_localize_script(
'edac-sidebar',
'edac_sidebar_app',
[
'gutenbergEnabled' => true,
'postID' => get_the_ID(),
'highlightNonce' => wp_create_nonce( 'edac_highlight' ),
'ajaxNonce' => wp_create_nonce( 'ajax-nonce' ),
'ajaxUrl' => admin_url( 'admin-ajax.php' ),
'edacApiUrl' => esc_url_raw( rest_url() . 'accessibility-checker/v1' ),
- 'nonce' => wp_create_nonce( 'wp_rest' ),
+ 'restNonce' => wp_create_nonce( 'wp_rest' ),
'settingsUrl' => esc_url_raw( admin_url( 'admin.php?page=accessibility_checker_settings' ) ),
'readabilityHelpUrl' => esc_url_raw( edac_link_wrapper( 'https://a11ychecker.com/help3265', 'wordpress-general', 'content-analysis-sidebar', false ) ),
'manuallyTestHelpUrl' => esc_url_raw( edac_link_wrapper( 'https://a11ychecker.com/help4280', 'wordpress-general', 'content-analysis-sidebar', false ) ),
]
);🤖 Prompt for AI Agents
In `@admin/class-enqueue-admin.php` around lines 195 - 211, Rename the REST API
nonce key in the wp_localize_script call that defines edac_sidebar_app from
'nonce' to 'restNonce' to match the project's convention (used elsewhere with
edac_script_vars and edac_editor_app); update the array key in the
wp_localize_script payload so the REST nonce is created with
wp_create_nonce('wp_rest') but exposed as 'restNonce', leaving other keys (like
'ajaxNonce') unchanged and ensuring all consumer code that reads
edac_sidebar_app expects restNonce.
| const data = await response.json(); | ||
|
|
||
| if ( ! data.success ) { | ||
| throw new Error( data.data?.message || __( 'Failed to dismiss issue', 'accessibility-checker' ) ); | ||
| } | ||
|
|
||
| return JSON.parse( data.data ); | ||
| }; |
There was a problem hiding this comment.
JSON.parse may throw on malformed response data.
If data.data is not valid JSON, JSON.parse(data.data) will throw an uncaught exception. Consider wrapping in try-catch or validating the response structure.
🛡️ Proposed fix
- return JSON.parse( data.data );
+ try {
+ return JSON.parse( data.data );
+ } catch {
+ throw new Error( __( 'Invalid response from server', 'accessibility-checker' ) );
+ }The same issue exists at line 131 in undismissIssue.
📝 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 data = await response.json(); | |
| if ( ! data.success ) { | |
| throw new Error( data.data?.message || __( 'Failed to dismiss issue', 'accessibility-checker' ) ); | |
| } | |
| return JSON.parse( data.data ); | |
| }; | |
| const data = await response.json(); | |
| if ( ! data.success ) { | |
| throw new Error( data.data?.message || __( 'Failed to dismiss issue', 'accessibility-checker' ) ); | |
| } | |
| try { | |
| return JSON.parse( data.data ); | |
| } catch { | |
| throw new Error( __( 'Invalid response from server', 'accessibility-checker' ) ); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@src/sidebar/components/IssueDetailsModal.js` around lines 91 - 98, The code
calls JSON.parse(data.data) in dismissIssue (and similarly in undismissIssue)
which will throw on malformed JSON; wrap the parse in a try-catch (or validate
that data.data is already an object) and on parse failure throw a new Error that
includes the raw response or parse error details so the exception is handled and
debuggable — e.g. in dismissIssue and undismissIssue, replace the direct
JSON.parse(data.data) with a guarded parse: try to parse, catch the SyntaxError,
and throw a descriptive Error containing the parse error and the original
data.data.
| const getViewOnPageUrl = ( issue, viewLink ) => { | ||
| const { highlightNonce } = window.edac_sidebar_app || {}; | ||
|
|
||
| if ( ! viewLink ) { | ||
| return null; | ||
| } | ||
|
|
||
| const url = new URL( viewLink ); | ||
| url.searchParams.set( 'edac', issue.id ); | ||
| if ( highlightNonce ) { | ||
| url.searchParams.set( 'edac_nonce', highlightNonce ); | ||
| } | ||
|
|
||
| return url.toString(); | ||
| }; |
There was a problem hiding this comment.
new URL(viewLink) can throw on invalid input.
If viewLink contains an invalid URL string, new URL(viewLink) will throw a TypeError. Consider wrapping in try-catch and returning null on failure.
🛡️ Proposed fix
const getViewOnPageUrl = ( issue, viewLink ) => {
const { highlightNonce } = window.edac_sidebar_app || {};
if ( ! viewLink ) {
return null;
}
+ let url;
+ try {
+ url = new URL( viewLink );
+ } catch {
+ return null;
+ }
- const url = new URL( viewLink );
url.searchParams.set( 'edac', issue.id );
if ( highlightNonce ) {
url.searchParams.set( 'edac_nonce', highlightNonce );
}
return url.toString();
};🤖 Prompt for AI Agents
In `@src/sidebar/components/RuleAccordion.js` around lines 20 - 34,
getViewOnPageUrl currently calls new URL(viewLink) which will throw a TypeError
for invalid viewLink strings; wrap the URL construction in a try-catch inside
getViewOnPageUrl, return null on any failure, and only manipulate searchParams
and return url.toString() when the URL was successfully created (preserve
behavior for highlightNonce and edac param). Ensure you reference
getViewOnPageUrl and handle the case where viewLink is falsy before attempting
URL creation.
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@admin/class-insert-rule-data.php`:
- Line 157: The array entry for the key 'ignre_comment' currently uses a ternary
that always returns null; update it to return the actual value from $rule_data
when present and null otherwise (use $rule_data['ignre_comment'] when set or the
null coalescing operator with $rule_data to default to null) so that
'ignre_comment' stores the incoming value instead of always being null.
- Around line 161-175: The code double-escapes the comment by calling esc_html()
around wp_kses(), which strips permitted tags; update the sanitization so that
if HTML is allowed you assign wp_kses( $rule_data['ignre_comment'],
$allowed_html ) directly to $rule_data_sanitized['ignre_comment'] (remove
esc_html), and if HTML should not be allowed replace the allowlist approach with
sanitize_textarea_field( $rule_data['ignre_comment'] ) instead; adjust the
branch around isset( $rule_data['ignre_comment'] ) and keep the $allowed_html
array and variable names as-is.
In `@includes/classes/class-rest-api.php`:
- Around line 959-972: The SQL currently interpolates $post_id directly into
$sql even though table and slugs are escaped; change the query to use
$wpdb->prepare() to bind the post ID (use a %d placeholder) before calling
$wpdb->get_results. Locate the variables $safe_table, $in_clause and $sql in
class-rest-api.php, replace the inline {$post_id} in the WHERE clause with a %d
placeholder and call $wpdb->prepare($sql, (int) $post_id) (keeping the
already-built $in_clause and escaped table name), then pass the prepared SQL to
$wpdb->get_results.
In `@includes/classes/Fixes/FixesManager.php`:
- Around line 289-291: The permission for the /fix-fields/{slug} endpoint is
inconsistent: it currently uses a hardcoded current_user_can('edit_posts') while
other endpoints use the edac_filter_settings_capability filter with a default of
'manage_options'; change the permission_callback for the register_route that
defines the /fix-fields handler to mirror the others by calling
current_user_can( apply_filters('edac_filter_settings_capability',
'manage_options') ) (or the equivalent helper used elsewhere in
FixesManager.php) so the capability is configurable and consistent with the
/fixes and /fixes/update endpoints.
In `@src/issueModal/components/DismissPanel.js`:
- Around line 24-30: The local state in DismissPanel (comment and isIgnored) is
only initialized from props and can become stale when issue changes; add a
useEffect that watches issue.id (or issue) and calls
setComment(decodeEntities(issue.ignre_comment) || '') and
setIsIgnored(issue?.ignre === '1' || issue?.ignre === 1) to resync when the prop
changes, leaving other state and handlers (dismissReason, isSubmitting, error,
successNotice) untouched; alternatively ensure parent forces remount via a key,
but preferred fix is adding useEffect inside DismissPanel to update comment and
isIgnored when issue.id changes.
In `@src/issueModal/components/FixPanel.js`:
- Around line 23-66: The useEffect in FixPanel.js (which defines fetchFixInfo
and calls setError/onError) closes over the onError prop but does not list it in
the dependency array; add onError to the dependency array for the useEffect that
defines fetchFixInfo (or ensure onError is stable via useCallback in the parent)
so the effect re-runs when onError changes and avoid a stale closure when
calling onError inside the try/catch blocks.
In `@src/issueModal/components/IssueDetailsModal.js`:
- Around line 172-181: The useSelect hook call that computes viewLink
(useSelect(...), referencing editorStore, getEditedPostPreviewLink,
getPermalink, isCurrentPostPublished) must be moved above the early return that
checks isOpen and issue so hooks are invoked unconditionally; relocate the
useSelect invocation to the top of the component (before the if (! isOpen || !
issue) return null) and keep the conditional return afterward, ensuring viewLink
remains available for use when rendering.
- Around line 143-162: The nested requestAnimationFrame in focusElement
reassigns rafId inside the outer callback which can leave the inner RAF
uncancelled; update focusElement to track both outer and inner IDs (e.g.,
outerRafId and innerRafId or an array of ids) when calling requestAnimationFrame
and ensure the cleanup returned by the effect calls cancelAnimationFrame on both
IDs (check for undefined) so neither the outer nor inner RAF can run after
unmount; reference modalRef, focusSection, focusElement, requestAnimationFrame
and cancelAnimationFrame to locate the change.
In `@src/issueModal/index.js`:
- Around line 114-116: closeIssueModal currently only resets modalState to
defaultState but doesn't run the teardown that actually hides/unmounts the
modal; update closeIssueModal to call the existing teardown logic (e.g., invoke
handleClose()) after resetting modalState so the component dispatches close
events and renderModal/unmount logic runs; ensure you reference and call
handleClose (and/or call renderModal to re-render the hidden state) from inside
closeIssueModal so the modal is removed from the DOM.
- Around line 58-67: Replace the deprecated render() usage in renderModal with
React 18's createRoot API: in renderModal (which calls ensureModalContainer and
renders IssueDetailsModal with modalState and onClose: handleClose) create a
root via createRoot(container) and call root.render(...) instead of render(...),
and ensure any unmount logic uses root.unmount() rather than render(null,
container); if needed add feature-detection fallback to preserve compatibility
with older WordPress versions.
In `@src/sidebar/components/Panels/AccessibilityStatus.js`:
- Around line 23-35: The useEffect in AccessibilityStatus sets a setTimeout in
handleIgnoreUpdated but doesn't clear it on unmount; modify the effect so it
stores the timeout id (e.g., in a ref or a variable scoped to the effect) when
calling window.setTimeout inside handleIgnoreUpdated, and call
clearTimeout(timeoutId) in the effect cleanup before removing the
'edac-ignore-updated' listener; ensure the stored id is cleared/reset each time
handleIgnoreUpdated runs to avoid stacked timeouts and race conditions around
refetch().
In `@src/sidebar/components/Panels/DismissedIssues.js`:
- Around line 37-40: Remove the unused emptyMessages prop: delete the
emptyMessages object declaration in DismissedIssues.js and stop passing
emptyMessages into the IssuesPanel component (the IssuesPanel call should no
longer include emptyMessages). Leave other props (title, initialOpen, tabs,
refreshing, showIgnored, className) unchanged so IssuesPanel receives only the
documented props.
In `@src/sidebar/components/Panels/ReadabilityAnalysis.js`:
- Around line 211-213: In ReadabilityAnalysis (the ReadabilityAnalysis
component) remove the href fallback pattern href={ settingsUrl || '#' } and
instead conditionally render the interactive anchor only when settingsUrl is
truthy; when settingsUrl is falsy render a non-interactive element (e.g., a
<span> with the same "edac-panel-section__link" class plus aria-disabled="true"
and tabIndex="-1") so the link is not keyboard-focusable or misleading. Update
every occurrence that uses settingsUrl (the anchor nodes with className
"edac-panel-section__link" around the settings text) at the mentioned spots so
anchors are only rendered with a real href, otherwise render the disabled span
variant.
🧹 Nitpick comments (13)
src/sidebar/utils/severityHelpers.js (1)
13-28: LGTM!Good use of WordPress i18n with the correct
'accessibility-checker'text domain. The fallback to empty string for unknown values is a safe default.For a minor optimization, consider defining
severityMapoutside the function to avoid recreating translated strings on each call, though the impact is negligible for typical usage patterns.src/issueModal/api.js (1)
16-26: LGTM!Clean API utility with sensible defaults. The conditional payload construction correctly excludes reason/comment when restoring an issue.
Minor: The
asynckeyword is unnecessary since the function just returns theapiFetchpromise directly without awaiting anything internally. Removing it would make the intent clearer, though the current implementation works correctly.-export const toggleIssueDismiss = async ( issueId, ignore = true, reason = '', comment = '' ) => { +export const toggleIssueDismiss = ( issueId, ignore = true, reason = '', comment = '' ) => {src/sidebar/sass/components/badge.scss (1)
8-88: LGTM!Well-structured SCSS with clear BEM naming, logical severity/type variants, and appropriate use of Sass
color.adjustfor semi-transparent backgrounds.Consider extracting the hardcoded text color
#1E1E1E(line 9) to a variable in_variables.scssfor consistency with other color tokens, especially if it's used elsewhere or may need theming support.src/sidebar/components/IssueImage.js (1)
17-99: Consider consolidating IssueImage to a shared module.This implementation duplicates
src/issueModal/components/IssueImage.js. A shared module would reduce drift and keep extraction logic in one place.src/sidebar/components/Panels/AccessibilityAnalysis.js (2)
14-16: Consider memoizing thedetailsobject reference.The
useMemohooks forproblemsandwarningsdepend ondetails, butdetailsis re-created as a new object on every render (data?.details || {}). This causes the memos to re-compute unnecessarily. Consider memoizingdetailsitself or usingdata?.detailsdirectly.♻️ Suggested optimization
- const details = data?.details || {}; - const problems = useMemo( () => details.errors || [], [ details ] ); - const warnings = useMemo( () => details.warnings || [], [ details ] ); + const problems = useMemo( () => data?.details?.errors || [], [ data?.details?.errors ] ); + const warnings = useMemo( () => data?.details?.warnings || [], [ data?.details?.warnings ] );
33-39: Consider adding a success state icon.The icon logic shows
errorwhen problems exist andwarningotherwise, but there's nocheckicon for when bothproblemCountand warnings are zero. This would provide clearer visual feedback for a fully accessible page.src/sidebar/components/Panels/ReadabilityAnalysis.js (1)
138-157: Consider simplifying icon determination logic.The
getPanelIconfunction has nested conditionals that are hard to follow. The logic at lines 145-155 checksreadingLevelStatus !== 'below'after already having a condition for=== 'below'on line 142, making the flow redundant.♻️ Simplified version
const getPanelIcon = () => { if ( ! hasContent || postGrade === 0 || postGrade === undefined || postGrade === null ) { return 'warning'; } if ( readingLevelStatus === 'below' ) { return 'check'; } - if ( readingLevelStatus !== 'below' ) { - if ( ! summaryText ) { - return 'warning'; - } - if ( summaryGrade > 0 && ! summaryGradeFailed ) { - return 'check'; - } - if ( summaryGradeFailed ) { - return 'warning'; - } + // readingLevelStatus is 'above' at this point + if ( ! summaryText || summaryGradeFailed ) { + return 'warning'; } + if ( summaryGrade > 0 ) { + return 'check'; + } return 'warning'; };src/sidebar/components/SidebarContent.js (1)
29-35: Consider type-checking the error before rendering.The
errorvalue is rendered directly in JSX. Iferrorcould be an object (e.g.,Errorinstance orWP_Error-like structure), this could cause rendering issues. Consider ensuring it's a string or extracting a message property.🛡️ Defensive rendering
if ( error ) { return ( <div className="edac-sidebar__error"> - <p>{ error }</p> + <p>{ typeof error === 'string' ? error : __( 'An error occurred loading accessibility data.', 'accessibility-checker' ) }</p> </div> ); }includes/classes/class-rest-api.php (1)
1086-1088: Double sanitization of summary field.The
summaryparameter is sanitized withsanitize_textarea_fieldin both the route args (line 258) and again here (line 1088). The second sanitization is redundant since REST API args sanitization runs before the callback.♻️ Remove redundant sanitization
public function save_simplified_summary( \WP_REST_Request $request ) { $post_id = (int) $request['id']; - $summary = sanitize_textarea_field( wp_unslash( $request['summary'] ) ); + $summary = $request['summary']; // Already sanitized by REST API args.tests/phpunit/includes/classes/RestApiSidebarDataTest.php (1)
152-206: Test relies on real rule registry which may be fragile.The
test_get_details_data_counts_and_passed_rulestest usesget_sample_rules()which fetches from the actual rule registry. This couples the test to the production rule definitions and may break if rules change. Consider usingmock_rules()for this test as well.src/issueModal/components/FixPanel.js (1)
278-287: Avoid using array index as key when items can be removed.Using
indexas the key for error notices can cause React reconciliation issues when errors are dismissed from the middle of the array. Consider using the error message itself as the key (if unique) or generating stable IDs.♻️ Proposed fix using error message as key
- { errors.map( ( error, index ) => ( + { errors.map( ( errorMsg ) => ( <Notice - key={ index } + key={ errorMsg } status="error" isDismissible={ true } - onRemove={ () => dismissError( index ) } + onRemove={ () => setErrors( ( prev ) => prev.filter( ( e ) => e !== errorMsg ) ) } > - { error } + { errorMsg } </Notice> ) ) }src/issueModal/components/RichTextarea.js (2)
178-179: Replace deprecatedonKeyPresswithonKeyDown.
onKeyPressis deprecated. UseonKeyDowninstead for consistent key event handling.♻️ Proposed fix
- onKeyPress={ ( e ) => e.key === 'Enter' && handleAddLink() } + onKeyDown={ ( e ) => { + if ( e.key === 'Enter' ) { + e.preventDefault(); + handleAddLink(); + } + } }
194-203: Add accessibility attributes to the contentEditable editor.The contentEditable div should have proper ARIA attributes for screen reader support, especially since this is an accessibility-focused plugin.
♻️ Proposed fix
<div ref={ editorRef } contentEditable={ ! disabled } suppressContentEditableWarning onInput={ handleInput } onKeyDown={ handleKeyDown } onBlur={ updateValue } className="edac-rich-textarea" style={ { minHeight: `${ rows * 24 }px` } } + role="textbox" + aria-multiline="true" + aria-label={ label } + aria-disabled={ disabled } />
| 'ignre_user' => isset( $rule_data['ignre_user'] ) ? absint( $rule_data['ignre_user'] ) : null, | ||
| 'ignre_date' => isset( $rule_data['ignre_date'] ) ? sanitize_text_field( $rule_data['ignre_date'] ) : null, | ||
| 'ignre_comment' => isset( $rule_data['ignre_comment'] ) ? sanitize_text_field( $rule_data['ignre_comment'] ) : null, | ||
| 'ignre_comment' => isset( $rule_data['ignre_comment'] ) ? null : null, |
There was a problem hiding this comment.
Redundant ternary expression always returns null.
The expression isset( $rule_data['ignre_comment'] ) ? null : null evaluates to null regardless of the condition. This appears to be incomplete code or an oversight.
🐛 Proposed fix
- 'ignre_comment' => isset( $rule_data['ignre_comment'] ) ? null : null,
+ 'ignre_comment' => null,📝 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.
| 'ignre_comment' => isset( $rule_data['ignre_comment'] ) ? null : null, | |
| 'ignre_comment' => null, |
🤖 Prompt for AI Agents
In `@admin/class-insert-rule-data.php` at line 157, The array entry for the key
'ignre_comment' currently uses a ternary that always returns null; update it to
return the actual value from $rule_data when present and null otherwise (use
$rule_data['ignre_comment'] when set or the null coalescing operator with
$rule_data to default to null) so that 'ignre_comment' stores the incoming value
instead of always being null.
| if ( isset( $rule_data['ignre_comment'] ) ) { | ||
| $allowed_html = [ | ||
| 'strong' => [], | ||
| 'b' => [], | ||
| 'em' => [], | ||
| 'i' => [], | ||
| 'a' => [ | ||
| 'href' => true, | ||
| 'target' => true, | ||
| 'rel' => true, | ||
| ], | ||
| ]; | ||
|
|
||
| $rule_data_sanitized['ignre_comment'] = esc_html( wp_kses( $rule_data['ignre_comment'], $allowed_html ) ); | ||
| } |
There was a problem hiding this comment.
Double-escaping issue: esc_html() wrapping wp_kses() negates the HTML allowlist.
wp_kses() returns sanitized HTML with permitted tags intact. Wrapping it with esc_html() converts all remaining HTML tags to entities, effectively stripping the allowed formatting (e.g., <strong>, <a>) that the allowlist is designed to preserve.
🐛 Proposed fix - use wp_kses alone if HTML is permitted
- $rule_data_sanitized['ignre_comment'] = esc_html( wp_kses( $rule_data['ignre_comment'], $allowed_html ) );
+ $rule_data_sanitized['ignre_comment'] = wp_kses( $rule_data['ignre_comment'], $allowed_html );If only plain text should be stored (no HTML), remove the allowlist and use sanitize_textarea_field() instead.
📝 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.
| if ( isset( $rule_data['ignre_comment'] ) ) { | |
| $allowed_html = [ | |
| 'strong' => [], | |
| 'b' => [], | |
| 'em' => [], | |
| 'i' => [], | |
| 'a' => [ | |
| 'href' => true, | |
| 'target' => true, | |
| 'rel' => true, | |
| ], | |
| ]; | |
| $rule_data_sanitized['ignre_comment'] = esc_html( wp_kses( $rule_data['ignre_comment'], $allowed_html ) ); | |
| } | |
| if ( isset( $rule_data['ignre_comment'] ) ) { | |
| $allowed_html = [ | |
| 'strong' => [], | |
| 'b' => [], | |
| 'em' => [], | |
| 'i' => [], | |
| 'a' => [ | |
| 'href' => true, | |
| 'target' => true, | |
| 'rel' => true, | |
| ], | |
| ]; | |
| $rule_data_sanitized['ignre_comment'] = wp_kses( $rule_data['ignre_comment'], $allowed_html ); | |
| } |
🤖 Prompt for AI Agents
In `@admin/class-insert-rule-data.php` around lines 161 - 175, The code
double-escapes the comment by calling esc_html() around wp_kses(), which strips
permitted tags; update the sanitization so that if HTML is allowed you assign
wp_kses( $rule_data['ignre_comment'], $allowed_html ) directly to
$rule_data_sanitized['ignre_comment'] (remove esc_html), and if HTML should not
be allowed replace the allowlist approach with sanitize_textarea_field(
$rule_data['ignre_comment'] ) instead; adjust the branch around isset(
$rule_data['ignre_comment'] ) and keep the $allowed_html array and variable
names as-is.
| // Build a simple, escaped IN clause. | ||
| $safe_table = esc_sql( $table_name ); | ||
| $escaped_slugs = array_map( 'esc_sql', $rule_slugs ); | ||
| $in_clause = "'" . implode( "','", $escaped_slugs ) . "'"; | ||
|
|
||
| // Direct SQL query (table and values already escaped). | ||
| $sql = "SELECT *\n" | ||
| . "FROM `{$safe_table}`\n" | ||
| . "WHERE postid = {$post_id}\n" | ||
| . "AND rule IN ( {$in_clause} )\n" | ||
| . "AND siteid = {$siteid}"; | ||
|
|
||
| // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching, WordPress.DB.PreparedSQL.NotPrepared | ||
| $all_results = $wpdb->get_results( $sql, ARRAY_A ); |
There was a problem hiding this comment.
SQL query should use $wpdb->prepare() for the post ID.
While the table name and rule slugs are escaped, $post_id is directly interpolated into the SQL string. Even though it's cast to int earlier, using $wpdb->prepare() is the WordPress standard for parameterized queries.
🔒 Recommended fix using prepare()
- // Build a simple, escaped IN clause.
- $safe_table = esc_sql( $table_name );
- $escaped_slugs = array_map( 'esc_sql', $rule_slugs );
- $in_clause = "'" . implode( "','", $escaped_slugs ) . "'";
-
- // Direct SQL query (table and values already escaped).
- $sql = "SELECT *\n"
- . "FROM `{$safe_table}`\n"
- . "WHERE postid = {$post_id}\n"
- . "AND rule IN ( {$in_clause} )\n"
- . "AND siteid = {$siteid}";
-
- // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching, WordPress.DB.PreparedSQL.NotPrepared
- $all_results = $wpdb->get_results( $sql, ARRAY_A );
+ // Build placeholders for IN clause.
+ $placeholders = implode( ',', array_fill( 0, count( $rule_slugs ), '%s' ) );
+
+ // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching
+ $all_results = $wpdb->get_results(
+ $wpdb->prepare(
+ "SELECT * FROM %i WHERE postid = %d AND rule IN ( $placeholders ) AND siteid = %d",
+ array_merge( [ $table_name, $post_id ], $rule_slugs, [ $siteid ] )
+ ),
+ ARRAY_A
+ );📝 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.
| // Build a simple, escaped IN clause. | |
| $safe_table = esc_sql( $table_name ); | |
| $escaped_slugs = array_map( 'esc_sql', $rule_slugs ); | |
| $in_clause = "'" . implode( "','", $escaped_slugs ) . "'"; | |
| // Direct SQL query (table and values already escaped). | |
| $sql = "SELECT *\n" | |
| . "FROM `{$safe_table}`\n" | |
| . "WHERE postid = {$post_id}\n" | |
| . "AND rule IN ( {$in_clause} )\n" | |
| . "AND siteid = {$siteid}"; | |
| // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching, WordPress.DB.PreparedSQL.NotPrepared | |
| $all_results = $wpdb->get_results( $sql, ARRAY_A ); | |
| // Build placeholders for IN clause. | |
| $placeholders = implode( ',', array_fill( 0, count( $rule_slugs ), '%s' ) ); | |
| // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching | |
| $all_results = $wpdb->get_results( | |
| $wpdb->prepare( | |
| "SELECT * FROM %i WHERE postid = %d AND rule IN ( $placeholders ) AND siteid = %d", | |
| array_merge( [ $table_name, $post_id ], $rule_slugs, [ $siteid ] ) | |
| ), | |
| ARRAY_A | |
| ); |
🤖 Prompt for AI Agents
In `@includes/classes/class-rest-api.php` around lines 959 - 972, The SQL
currently interpolates $post_id directly into $sql even though table and slugs
are escaped; change the query to use $wpdb->prepare() to bind the post ID (use a
%d placeholder) before calling $wpdb->get_results. Locate the variables
$safe_table, $in_clause and $sql in class-rest-api.php, replace the inline
{$post_id} in the WHERE clause with a %d placeholder and call
$wpdb->prepare($sql, (int) $post_id) (keeping the already-built $in_clause and
escaped table name), then pass the prepared SQL to $wpdb->get_results.
| 'permission_callback' => function () { | ||
| return current_user_can( 'edit_posts' ); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's verify the actual permissions in FixesManager.php
# Check lines around 260, 272, and 290 for permission callbacks
echo "=== Checking permission callbacks in FixesManager.php ==="
rg -n "permission_callback" includes/classes/Fixes/FixesManager.php -A2 -B1
# Also search for the route registrations to understand context
echo -e "\n=== Checking route registrations ==="
rg -n "register_rest_route.*fix" includes/classes/Fixes/FixesManager.php -B2 -A5Repository: equalizedigital/accessibility-checker
Length of output: 753
🏁 Script executed:
#!/bin/bash
# Now let's search for how fix-fields endpoint is consumed
echo "=== Searching for fix-fields usage ==="
rg -n "fix-fields" --type js --type php -C3
# Also check for any documentation or comments about permissions
echo -e "\n=== Searching for related permission contexts ==="
rg -n "edit_posts|manage_options" includes/classes/Fixes/FixesManager.php -B1 -A1Repository: equalizedigital/accessibility-checker
Length of output: 1522
🏁 Script executed:
#!/bin/bash
# Check what capabilities are typically required for similar endpoints
echo "=== Checking all REST endpoint registrations in the file ==="
rg -n "register_rest_route" includes/classes/Fixes/FixesManager.php -A10Repository: equalizedigital/accessibility-checker
Length of output: 1277
Permission level inconsistency between fix endpoints.
The new /fix-fields/{slug} endpoint uses edit_posts capability (line 290), while the existing /fixes and /fixes/update endpoints use manage_options (lines 260, 272). Additionally, the /fix-fields endpoint hardcodes edit_posts without applying the edac_filter_settings_capability filter used by other endpoints, preventing overrides. This allows editors to access fix field data while restricting configuration to admins—clarify if this permission split is intentional.
🤖 Prompt for AI Agents
In `@includes/classes/Fixes/FixesManager.php` around lines 289 - 291, The
permission for the /fix-fields/{slug} endpoint is inconsistent: it currently
uses a hardcoded current_user_can('edit_posts') while other endpoints use the
edac_filter_settings_capability filter with a default of 'manage_options';
change the permission_callback for the register_route that defines the
/fix-fields handler to mirror the others by calling current_user_can(
apply_filters('edac_filter_settings_capability', 'manage_options') ) (or the
equivalent helper used elsewhere in FixesManager.php) so the capability is
configurable and consistent with the /fixes and /fixes/update endpoints.
| const DismissPanel = ( { issue, isOpen, onToggle, onIgnore } ) => { | ||
| const [ comment, setComment ] = useState( issue?.ignre_comment ? decodeEntities( issue.ignre_comment ) : '' ); | ||
| const [ dismissReason, setDismissReason ] = useState( 'false_positive' ); | ||
| const [ isSubmitting, setIsSubmitting ] = useState( false ); | ||
| const [ error, setError ] = useState( null ); | ||
| const [ successNotice, setSuccessNotice ] = useState( null ); | ||
| const [ isIgnored, setIsIgnored ] = useState( issue?.ignre === '1' || issue?.ignre === 1 ); |
There was a problem hiding this comment.
State may become stale when issue prop changes.
The comment and isIgnored states are initialized from the issue prop but won't update if the component receives a different issue while mounted. Consider using useEffect to sync state when issue.id changes, or use a key prop on the parent to force remount.
🔧 Proposed fix - sync state when issue changes
+import { useState, useEffect } from '@wordpress/element';
const DismissPanel = ( { issue, isOpen, onToggle, onIgnore } ) => {
const [ comment, setComment ] = useState( issue?.ignre_comment ? decodeEntities( issue.ignre_comment ) : '' );
const [ dismissReason, setDismissReason ] = useState( 'false_positive' );
const [ isSubmitting, setIsSubmitting ] = useState( false );
const [ error, setError ] = useState( null );
const [ successNotice, setSuccessNotice ] = useState( null );
const [ isIgnored, setIsIgnored ] = useState( issue?.ignre === '1' || issue?.ignre === 1 );
+ // Reset state when issue changes
+ useEffect( () => {
+ setComment( issue?.ignre_comment ? decodeEntities( issue.ignre_comment ) : '' );
+ setIsIgnored( issue?.ignre === '1' || issue?.ignre === 1 );
+ setError( null );
+ setSuccessNotice( null );
+ }, [ issue?.id ] );🤖 Prompt for AI Agents
In `@src/issueModal/components/DismissPanel.js` around lines 24 - 30, The local
state in DismissPanel (comment and isIgnored) is only initialized from props and
can become stale when issue changes; add a useEffect that watches issue.id (or
issue) and calls setComment(decodeEntities(issue.ignre_comment) || '') and
setIsIgnored(issue?.ignre === '1' || issue?.ignre === 1) to resync when the prop
changes, leaving other state and handlers (dismissReason, isSubmitting, error,
successNotice) untouched; alternatively ensure parent forces remount via a key,
but preferred fix is adding useEffect inside DismissPanel to update comment and
isIgnored when issue.id changes.
| const renderModal = () => { | ||
| const container = ensureModalContainer(); | ||
| render( | ||
| createElement( IssueDetailsModal, { | ||
| ...modalState, | ||
| onClose: handleClose, | ||
| } ), | ||
| container, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WordPress React 18 compatibility createRoot migration 2025
💡 Result:
WordPress has bundled React 18 since WordPress 6.2, so in 2025+ your plugin/theme/admin UIs that mount React into the page should be React 18–compatible and ideally migrate off ReactDOM.render to createRoot. (make.wordpress.org)
What to change (React 18 / WordPress way)
Legacy (still works, but warns and runs “React 17 legacy mode”)
import { render } from '@wordpress/element';
render( <App />, document.getElementById('root') );WordPress core notes this continues to work, but you’ll see the React 18 console warning and you won’t get concurrent features. (make.wordpress.org)
Recommended (React 18)
import { createRoot } from '@wordpress/element';
const el = document.getElementById('root');
const root = createRoot(el);
root.render(<App />);Unmounting also changes from unmountComponentAtNode(el) to:
root.unmount();Backward compatibility strategy (if you still support WP < 6.2)
@wordpress/element documents a practical pattern: import both and fallback to render when createRoot isn’t available. (npmjs.com)
import { createRoot, render } from '@wordpress/element';
const el = document.getElementById('root');
if ( createRoot ) {
createRoot(el).render(<App />);
} else {
render(<App />, el);
}Why you’re seeing warnings in WP admin/editor
If any plugin/theme code calls ReactDOM.render (directly, or via older tooling), React 18 logs: “ReactDOM.render is no longer supported in React 18. Use createRoot instead.”—commonly reported after WP updates because WordPress ships React 18. (wordpress.org)
Citations:
- 1: https://make.wordpress.org/core/2023/03/07/upgrading-to-react-18-and-common-pitfalls-of-concurrent-mode/?utm_source=openai
- 2: https://make.wordpress.org/core/2023/03/07/upgrading-to-react-18-and-common-pitfalls-of-concurrent-mode/?utm_source=openai
- 3: https://make.wordpress.org/core/2023/03/07/upgrading-to-react-18-and-common-pitfalls-of-concurrent-mode/?utm_source=openai
- 4: https://www.npmjs.com/package/%40wordpress/element/v/5.18.0?utm_source=openai
- 5: https://wordpress.org/support/topic/warning-reactdom-render-is-no-longer-supported-in-react-18/?utm_source=openai
🏁 Script executed:
# Check package.json and dependencies to understand React version
fd -t f package.json | head -5 | xargs catRepository: equalizedigital/accessibility-checker
Length of output: 2868
🏁 Script executed:
# Search for react and wordpress package versions in lock files
fd -t f -e lock.json -e lock.yaml | head -3 | xargs ls -lhRepository: equalizedigital/accessibility-checker
Length of output: 1884
🏁 Script executed:
# Check the specific file and its imports
cat -n src/issueModal/index.js | head -80Repository: equalizedigital/accessibility-checker
Length of output: 2684
🏁 Script executed:
# Search for other uses of render() from `@wordpress/element`
rg "render\(" --type js -A 2 -B 2 | head -100Repository: equalizedigital/accessibility-checker
Length of output: 617
🏁 Script executed:
# Check if unmountComponentAtNode is used anywhere
rg "unmountComponentAtNode" --type jsRepository: equalizedigital/accessibility-checker
Length of output: 63
Migrate from deprecated render() to React 18's createRoot() API.
WordPress has bundled React 18 since version 6.2, and the legacy render() function from @wordpress/element is deprecated. This code will generate React 18 console warnings and won't benefit from concurrent rendering features. Migrate to createRoot() following the WordPress migration guide:
Suggested refactor pattern
import { createRoot, createElement } from '@wordpress/element';
const renderModal = () => {
const container = ensureModalContainer();
const root = createRoot(container);
root.render(
createElement(IssueDetailsModal, {
...modalState,
onClose: handleClose,
})
);
};
// For unmounting, use root.unmount() instead of render(null, container)If backward compatibility with WordPress < 6.2 is required, use a feature detection fallback.
🤖 Prompt for AI Agents
In `@src/issueModal/index.js` around lines 58 - 67, Replace the deprecated
render() usage in renderModal with React 18's createRoot API: in renderModal
(which calls ensureModalContainer and renders IssueDetailsModal with modalState
and onClose: handleClose) create a root via createRoot(container) and call
root.render(...) instead of render(...), and ensure any unmount logic uses
root.unmount() rather than render(null, container); if needed add
feature-detection fallback to preserve compatibility with older WordPress
versions.
| const closeIssueModal = () => { | ||
| modalState = { ...defaultState }; | ||
| }; |
There was a problem hiding this comment.
closeIssueModal doesn't trigger the actual close logic.
The closeIssueModal function only resets modalState but doesn't call handleClose() or renderModal(). The modal will remain visible in the DOM. Compare with handleClose which properly dispatches events and unmounts the component.
🐛 Proposed fix
const closeIssueModal = () => {
- modalState = { ...defaultState };
+ handleClose();
};🤖 Prompt for AI Agents
In `@src/issueModal/index.js` around lines 114 - 116, closeIssueModal currently
only resets modalState to defaultState but doesn't run the teardown that
actually hides/unmounts the modal; update closeIssueModal to call the existing
teardown logic (e.g., invoke handleClose()) after resetting modalState so the
component dispatches close events and renderModal/unmount logic runs; ensure you
reference and call handleClose (and/or call renderModal to re-render the hidden
state) from inside closeIssueModal so the modal is removed from the DOM.
| useEffect( () => { | ||
| const handleIgnoreUpdated = () => { | ||
| // Small delay so the ignore save can complete before we refetch. | ||
| window.setTimeout( () => { | ||
| refetch(); | ||
| }, 300 ); | ||
| }; | ||
|
|
||
| window.addEventListener( 'edac-ignore-updated', handleIgnoreUpdated ); | ||
|
|
||
| return () => { | ||
| window.removeEventListener( 'edac-ignore-updated', handleIgnoreUpdated ); | ||
| }; |
There was a problem hiding this comment.
Clear pending refetch timeouts on unmount.
The queued timeout can still run after unmount or stack up across rapid ignore events. Track the timeout id and clear it in cleanup.
🛠️ Suggested fix
useEffect( () => {
- const handleIgnoreUpdated = () => {
- // Small delay so the ignore save can complete before we refetch.
- window.setTimeout( () => {
- refetch();
- }, 300 );
- };
+ let timeoutId;
+ const handleIgnoreUpdated = () => {
+ // Small delay so the ignore save can complete before we refetch.
+ window.clearTimeout( timeoutId );
+ timeoutId = window.setTimeout( () => {
+ refetch();
+ }, 300 );
+ };
window.addEventListener( 'edac-ignore-updated', handleIgnoreUpdated );
return () => {
window.removeEventListener( 'edac-ignore-updated', handleIgnoreUpdated );
+ window.clearTimeout( timeoutId );
};
}, [ refetch ] );🤖 Prompt for AI Agents
In `@src/sidebar/components/Panels/AccessibilityStatus.js` around lines 23 - 35,
The useEffect in AccessibilityStatus sets a setTimeout in handleIgnoreUpdated
but doesn't clear it on unmount; modify the effect so it stores the timeout id
(e.g., in a ref or a variable scoped to the effect) when calling
window.setTimeout inside handleIgnoreUpdated, and call clearTimeout(timeoutId)
in the effect cleanup before removing the 'edac-ignore-updated' listener; ensure
the stored id is cleared/reset each time handleIgnoreUpdated runs to avoid
stacked timeouts and race conditions around refetch().
… relevant sections in AccessibilityAnalysis
…lity-status-link-stats-to-their-panel Add: enable clickable cards in AccessibilityStatus to scroll and open accordions
…sibility-analysis-count Add accessibility issues counts and improve screen reader text
* Features: * - Initial load vs background refresh states * - Shallow data comparison to prevent unnecessary re-renders * - UI state management * - Debounced refresh to prevent rapid successive updates
…d unwanted re-renders
…d emptyMessages in AccessibilityAnalysis and DismissedIssues Style: add padding to message styling in accessibility-analysis.scss
…ismissed it will focus on the rule header
…n-branch-for-ac-sidebar' into william/pro-526-setup-integration-branch-for-ac-sidebar # Conflicts: # src/issueModal/components/RichTextarea.js
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
[PRO-610]
…ta1-issue-actions-button-ambiguous Add the issue ID to the label for the issue actions menu
This change was previously lost when adding a shortcut
…al metadata display
…odal-style-pass-on-dismissed-issues Enhance: DismissPanel styling and functionality with improved dismiss…
…ta1-dismiss-issue-text-area-is-unlabelled-and-missing Add aria-labelledby and aria-describedby to content input for dismiss comment
This keep the component reusable incase we use it elsewhere on the page
…n-branch-for-ac-sidebar' into william/pro-526-setup-integration-branch-for-ac-sidebar # Conflicts: # src/issueModal/components/RichTextarea.js
Not if greater than or equal to 9 [PRO-618]
…nd improved theming
…ta1-discrepancies-between-sidebar-and-meta-box Prompt for summary when ABOVE 9th grade, not at it
… are Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ied-summaries-reading-level-cant-be-calculated-issues Add a state in the classic metabox for when there is no reading grade able to be calulated
…efault-admin-color Update color variables to use CSS custom properties from admin theme
|
✅ Accessibility Checker build (primary only)
|
This is the integration branch where the work for the sidebar will be merged into to prep for testing and release. Notes will be added as it progress.
Summary by CodeRabbit
New Features
Tests
Chores