Add scan-to-scan accessibility regression detection and sidebar trend messaging#1588
Add scan-to-scan accessibility regression detection and sidebar trend messaging#1588SteveJonesDev wants to merge 2 commits into
Conversation
Summary of ChangesHello, 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 significantly enhances the accessibility checker by introducing scan-to-scan regression detection and dynamic trend messaging in the sidebar. The core purpose is to provide users with immediate, actionable feedback on how accessibility is evolving over time, highlighting declines and encouraging continuous monitoring. This is achieved by comparing current scan results with previous ones, storing a history of summaries, and updating the user interface to display insightful trends rather than static placeholders. Highlights
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. Footnotes
|
📝 WalkthroughWalkthroughAdds regression tracking: backend computes per-scan deltas, status, and history; REST API always returns a Changes
Sequence Diagram(s)sequenceDiagram
participant Scan as Accessibility Scan
participant Gen as Summary Generator
participant Meta as Post Meta Storage
participant REST as REST API
participant UI as Sidebar UI
Scan->>Gen: provide current metrics
Gen->>Meta: read `_edac_summary` (previous)
Gen->>Gen: issue_counts_match(previous, current)?
alt counts match & previous regression exists
Gen->>Gen: reuse previous regression
else
Gen->>Gen: build_regression_data(previous, current)
end
Gen->>Meta: save `_edac_summary` (includes regression)
Gen->>Meta: update `_edac_summary_history` (append, dedupe, truncate to 10)
UI->>REST: GET summary
REST->>Meta: fetch `_edac_summary`
REST->>REST: ensure `regression` present (defaults if missing)
REST-->>UI: return summary with `regression`
UI->>UI: renderRegressionMessage(delta, status)
UI-->>User: display trend/regression messages and retention row if declining
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0638fd84a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( $regression_score >= 5 || $deltas['passed_tests'] <= -10 ) { | ||
| $status = 'declining'; | ||
| } elseif ( $regression_score > 0 || $deltas['passed_tests'] < 0 ) { | ||
| $status = 'watch'; | ||
| } elseif ( $deltas['errors'] < 0 || $deltas['warnings'] < 0 || $deltas['contrast_errors'] < 0 || $deltas['passed_tests'] > 0 ) { |
There was a problem hiding this comment.
This classification runs even when there is no prior summary, so a first-ever scan with existing errors/warnings is labeled watch/declining despite has_baseline being false. The sidebar then uses that status for retention messaging, which can incorrectly tell users accessibility is declining before any scan-to-scan comparison exists. Please short-circuit to stable (or equivalent neutral status) until a baseline is present.
Useful? React with 👍 / 👎.
![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 accessibility regression tracking feature. The backend changes involve modifying the REST API to include a new regression data structure in the summary, which contains details like has_baseline, status, delta for errors, warnings, contrast errors, and passed tests, along with a scanned_at timestamp. A new build_regression_data method is added to calculate these deltas and determine a regression status (stable, declining, watch, improving) by comparing the current scan with the previous one. Additionally, a update_summary_history method is implemented to store a compact history of the last 10 summaries for trend analysis, and the sanitization process is updated for the new regression data. On the frontend, the AccessibilityStatus component is updated to display these new regression metrics, showing trend messages for problems, warnings, and passed checks, and a retention message based on the overall regression status. Corresponding SCSS styles are added for the new UI elements. Unit tests have been updated and added to cover the initialization and calculation of the regression data.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
includes/classes/class-rest-api.php (1)
877-900: LGTM! Consider extracting the default regression structure to reduce duplication.The logic correctly ensures backward compatibility by always providing a
regressionobject. The two code paths (lines 877-888 and 890-900) use identical default structures.,
♻️ Optional: Extract default structure to reduce duplication
private function get_summary_data( $post_id ) { // Get summary from post meta. $summary = get_post_meta( $post_id, '_edac_summary', true ); + + $default_regression = [ + 'has_baseline' => false, + 'status' => 'stable', + 'delta' => [ + 'errors' => 0, + 'warnings' => 0, + 'contrast_errors' => 0, + 'passed_tests' => 0, + ], + 'scanned_at' => '', + ]; // If summary doesn't exist or is invalid, return defaults. if ( ! $summary || ! is_array( $summary ) ) { $summary = [ 'passed_tests' => 0, 'errors' => 0, 'contrast_errors' => 0, 'warnings' => 0, 'ignored' => 0, 'readability' => 0, - 'regression' => [ - 'has_baseline' => false, - 'status' => 'stable', - 'delta' => [ - 'errors' => 0, - 'warnings' => 0, - 'contrast_errors' => 0, - 'passed_tests' => 0, - ], - 'scanned_at' => '', - ], + 'regression' => $default_regression, ]; } elseif ( ! isset( $summary['regression'] ) || ! is_array( $summary['regression'] ) ) { - $summary['regression'] = [ - 'has_baseline' => false, - 'status' => 'stable', - 'delta' => [ - 'errors' => 0, - 'warnings' => 0, - 'contrast_errors' => 0, - 'passed_tests' => 0, - ], - 'scanned_at' => '', - ]; + $summary['regression'] = $default_regression; } return $summary; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/classes/class-rest-api.php` around lines 877 - 900, Extract the duplicated default regression array into a single reusable symbol (e.g., a private method get_default_regression() or a private const DEFAULT_REGRESSION) and replace both inline arrays that set $summary['regression'] with a call/reference to that symbol; ensure the new symbol returns the same structure (has_baseline, status, delta with errors/warnings/contrast_errors/passed_tests, scanned_at) so both the initial assignment and the elseif branch use the centralized default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@includes/classes/class-summary-generator.php`:
- Around line 109-110: Remove the manual alignment of the assignment operators
causing the PHPCS warning by normalizing spacing for the two assignments: change
the lines that set $current_value and $previous_value (which reference
$current_summary, $previous_summary and call absint()) so they use a single
space around the = operator instead of padded alignment; keep the same logic
(use the null-coalescing/default and is_array check) but ensure spacing is
consistent with PSR/PHPCS rules.
---
Nitpick comments:
In `@includes/classes/class-rest-api.php`:
- Around line 877-900: Extract the duplicated default regression array into a
single reusable symbol (e.g., a private method get_default_regression() or a
private const DEFAULT_REGRESSION) and replace both inline arrays that set
$summary['regression'] with a call/reference to that symbol; ensure the new
symbol returns the same structure (has_baseline, status, delta with
errors/warnings/contrast_errors/passed_tests, scanned_at) so both the initial
assignment and the elseif branch use the centralized default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 02d72660-524b-4a93-a2a3-55c5ec19e03f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
includes/classes/class-rest-api.phpincludes/classes/class-summary-generator.phpsrc/sidebar/components/Panels/AccessibilityStatus.jssrc/sidebar/sass/components/accessibility-status.scsstests/phpunit/includes/classes/RestApiSidebarDataTest.phptests/phpunit/includes/classes/SummaryGeneratorTest.php
| $current_value = absint( $current_summary[ $metric ] ?? 0 ); | ||
| $previous_value = is_array( $previous_summary ) ? absint( $previous_summary[ $metric ] ?? 0 ) : 0; |
There was a problem hiding this comment.
Fix PHPCS alignment warning.
The pipeline flagged an alignment issue with the equals signs in these assignments.
🔧 Proposed fix
foreach ( $metrics as $metric ) {
- $current_value = absint( $current_summary[ $metric ] ?? 0 );
- $previous_value = is_array( $previous_summary ) ? absint( $previous_summary[ $metric ] ?? 0 ) : 0;
+ $current_value = absint( $current_summary[ $metric ] ?? 0 );
+ $previous_value = is_array( $previous_summary ) ? absint( $previous_summary[ $metric ] ?? 0 ) : 0;
$deltas[ $metric ] = $current_value - $previous_value;
}🧰 Tools
🪛 GitHub Actions: CS
[warning] 109-110: PHPCS Generic.Formatting.MultipleStatementAlignment.NotSameWarning: Equals sign not aligned with surrounding assignments; expected 5 spaces but found 1 space / expected 4 spaces but found 1 space.
🪛 GitHub Check: Check code style
[warning] 110-110:
Equals sign not aligned with surrounding assignments; expected 4 spaces but found 1 space
[warning] 109-109:
Equals sign not aligned with surrounding assignments; expected 5 spaces but found 1 space
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/classes/class-summary-generator.php` around lines 109 - 110, Remove
the manual alignment of the assignment operators causing the PHPCS warning by
normalizing spacing for the two assignments: change the lines that set
$current_value and $previous_value (which reference $current_summary,
$previous_summary and call absint()) so they use a single space around the =
operator instead of padded alignment; keep the same logic (use the
null-coalescing/default and is_array check) but ensure spacing is consistent
with PSR/PHPCS rules.
…s match and include content grade in summary updates
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@includes/classes/class-summary-generator.php`:
- Around line 443-453: The current comparison uses absint(... ?? -1) which turns
a missing value into 1 and can falsely match real values; update the block that
checks $last vs $new_entry so you first ensure each metric exists (use
isset($last['errors']) / isset($new_entry['errors']) etc.) and then compare
their integer values (e.g., absint($last['errors']) ===
absint($new_entry['errors'])) only when both exist, otherwise treat as
different; apply this pattern to errors, warnings, contrast_errors, passed_tests
and content_grade to avoid the -1 fallback ambiguity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 910f48ea-90a6-4b1a-b78c-166cecd007fa
📒 Files selected for processing (2)
includes/classes/class-summary-generator.phpsrc/sidebar/components/Panels/AccessibilityStatus.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sidebar/components/Panels/AccessibilityStatus.js
| if ( ! empty( $history ) ) { | ||
| $last = end( $history ); | ||
| if ( | ||
| absint( $last['errors'] ?? -1 ) === $new_entry['errors'] && | ||
| absint( $last['warnings'] ?? -1 ) === $new_entry['warnings'] && | ||
| absint( $last['contrast_errors'] ?? -1 ) === $new_entry['contrast_errors'] && | ||
| absint( $last['passed_tests'] ?? -1 ) === $new_entry['passed_tests'] && | ||
| absint( $last['content_grade'] ?? -1 ) === $new_entry['content_grade'] | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Default value with absint() can cause false matches.
Using -1 as a fallback with absint() yields 1, which could incorrectly match a legitimate entry where the metric equals 1—causing that entry to be skipped.
Consider using a direct comparison without the absint() wrapper on the fallback, or restructure to avoid the ambiguity:
🛡️ Suggested fix
if (
- absint( $last['errors'] ?? -1 ) === $new_entry['errors'] &&
- absint( $last['warnings'] ?? -1 ) === $new_entry['warnings'] &&
- absint( $last['contrast_errors'] ?? -1 ) === $new_entry['contrast_errors'] &&
- absint( $last['passed_tests'] ?? -1 ) === $new_entry['passed_tests'] &&
- absint( $last['content_grade'] ?? -1 ) === $new_entry['content_grade']
+ isset( $last['errors'], $last['warnings'], $last['contrast_errors'], $last['passed_tests'], $last['content_grade'] ) &&
+ absint( $last['errors'] ) === $new_entry['errors'] &&
+ absint( $last['warnings'] ) === $new_entry['warnings'] &&
+ absint( $last['contrast_errors'] ) === $new_entry['contrast_errors'] &&
+ absint( $last['passed_tests'] ) === $new_entry['passed_tests'] &&
+ absint( $last['content_grade'] ) === $new_entry['content_grade']
) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/classes/class-summary-generator.php` around lines 443 - 453, The
current comparison uses absint(... ?? -1) which turns a missing value into 1 and
can falsely match real values; update the block that checks $last vs $new_entry
so you first ensure each metric exists (use isset($last['errors']) /
isset($new_entry['errors']) etc.) and then compare their integer values (e.g.,
absint($last['errors']) === absint($new_entry['errors'])) only when both exist,
otherwise treat as different; apply this pattern to errors, warnings,
contrast_errors, passed_tests and content_grade to avoid the -1 fallback
ambiguity.
1 participant
Motivation
Description
Summary_Generatorby comparing the current scan summary with the previously saved summary and producing aregressionpayload containingstatus,delta,has_baseline, andscanned_atviabuild_regression_data(new private method) and addedregressioninto the sanitized summary output insanitize_summary_meta_data(file:includes/classes/class-summary-generator.php)._edac_summary_history) for up to 10 scans to support trend messaging viaupdate_summary_history(new private method) and continue writing the primary_edac_summarymeta (file:includes/classes/class-summary-generator.php).regressionobject (defaults provided) so older summaries and UI consumers remain compatible (file:includes/classes/class-rest-api.php).declining, and added styles to support the new microcopy (files:src/sidebar/components/Panels/AccessibilityStatus.js,src/sidebar/sass/components/accessibility-status.scss).tests/phpunit/includes/classes/RestApiSidebarDataTest.php,tests/phpunit/includes/classes/SummaryGeneratorTest.php).package-lock.jsonversion metadata to match package version.Testing
npm run lint:js -- src/sidebar/components/Panels/AccessibilityStatus.js, which completed successfully.php -lon modified PHP files:includes/classes/class-summary-generator.php,includes/classes/class-rest-api.php, and related test files, which returned no syntax errors.vendor/bin/phpunit tests/phpunit/includes/classes/SummaryGeneratorTest.php tests/phpunit/includes/classes/RestApiSidebarDataTest.php, however the environment is missing the WordPress test library (/tmp/wordpress-tests-lib/includes/functions.php) so full test execution could not be completed in this run (the unit test assertions were added and verify expected behavior when run in CI or a properly provisioned local test environment).Codex Task
Summary by CodeRabbit
New Features
Style
Tests