Make scan runnable from frontend loaded page#979
Conversation
…the old scan runner in iframe checking contexts
WalkthroughThe code refactors scan initialization and timeout management in the page scanner module. It introduces global timeout handling, utility functions for iframe context detection and option extraction, and exposes a global method for manual scan triggering. The scan and cleanup logic is updated for safer DOM querying and more explicit error reporting. Additionally, the frontend highlighter app integrates a fallback scan-and-save workflow triggered by specific AJAX response codes, with dynamic script loading, retry logic, and enhanced UI error handling. The enqueue frontend class adds a script URL for the scanner bundle. Changes
Sequence Diagram(s)sequenceDiagram
participant Window
participant PageScanner
participant DOM
participant FrontendApp
participant Server
Window->>PageScanner: runAccessibilityScan(options, onDone)
alt In iframe context
PageScanner->>DOM: Extract iframe options
PageScanner->>PageScanner: Start scan with options
PageScanner->>Window: Set timeout for scan
PageScanner->>PageScanner: On scan complete or fail, call onDone
else Manual scan
PageScanner->>PageScanner: Start scan with provided options
PageScanner->>PageScanner: On scan complete or fail, call onDone
end
PageScanner->>Window: Dispatch done event with error code (if any)
FrontendApp->>Server: AJAX call to get scan results
Server-->>FrontendApp: Response with error code -3 (no results)
FrontendApp->>FrontendApp: kickoffScan()
FrontendApp->>Window: runAccessibilityScan()
Window-->>FrontendApp: Scan results
FrontendApp->>Server: saveScanResults()
Server-->>FrontendApp: Save confirmation
FrontendApp->>FrontendApp: Retry AJAX call after 5s
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)src/frontendHighlighterApp/index.js (1)🧬 Code Graph Analysis (1)src/frontendHighlighterApp/index.js (2)
🪛 Biome (1.9.4)src/frontendHighlighterApp/index.js[error] 952-953: Change to an optional chain. Unsafe fix: Change to an optional chain. (lint/complexity/useOptionalChain) [error] 953-954: Change to an optional chain. Unsafe fix: Change to an optional chain. (lint/complexity/useOptionalChain) [error] 989-989: Change to an optional chain. Unsafe fix: Change to an optional chain. (lint/complexity/useOptionalChain) [error] 945-946: This aliasing of this is unnecessary. Arrow functions inherits (lint/complexity/noUselessThisAlias) [error] 972-973: This aliasing of this is unnecessary. Arrow functions inherits (lint/complexity/noUselessThisAlias) ⏰ 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)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @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!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request. The main goal of this PR, as described in the title and description, is to make the pageScanner bundle runnable directly from a frontend page via the window object. Previously, it seemed primarily designed to run within an iframe context. This change refactors the scanner's entry point to support both the original iframe-based execution and a new direct execution mode, allowing for rescans on a loaded page if needed. It also includes some minor robustness improvements.
Highlights
- Expose Scanner to Window: The core change is the addition of a
window.runAccessibilityScanfunction. This allows consumers to trigger the accessibility scan directly from the main window context, passing options and receiving results via a Promise and optionalonCompletecallback. - Refactor Execution Flow: The logic for automatically starting the scan and dispatching results via events is now conditional. It only runs if the script detects it is loaded within the expected iframe context, using new helper functions
isIframeContextandgetIframeOptions. - Improved Element Selection Robustness: Added checks before accessing the
outerHTMLproperty of elements found viadocument.querySelector. If an element cannot be found (e.g., due to dynamic changes), it will now assignnullto thehtmlproperty in the results instead of potentially throwing an error. - Adjusted Error Reporting: The
onDonefunction, used for finalizing the scan and dispatching results, has been updated to pass more specific string identifiers for different error conditions (like timeout, cleanup failure, or missing cleanup) instead of a generic boolean flag.
Changelog
- src/pageScanner/index.js
- Added
tooLongTimeoutvariable declaration outside the main execution block (line 14). - Introduced
isIframeContext()function to check if the script is running in an iframe (lines 22-29). - Introduced
getIframeOptions()function to extract iframe-related data attributes (lines 31-43). - Added null check for
document.querySelectorresults before accessingouterHTMLwhen processing violations (lines 88, 91). - Added null check for
document.querySelectorresults before accessingouterHTMLwhen processing incomplete results for 'form-field-multiple-labels' (lines 102, 105). - Modified
onDoneto pass specific string error types ('', 'cleanup-failed', 'cleanup-not-exists') instead of a boolean (lines 185, 191, 197). - Removed the immediate auto-execution of
scan()and the initial timeout setup. - Added
window.runAccessibilityScanfunction to expose the scan functionality to the global window object (lines 201-221). - Wrapped the auto-execution logic and timeout setup within an
if ( isIframeContext() )block (lines 224-235).
- Added
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
A scanner in a frame,
Now runs outside the game.
Onwindowit waits,
To check accessibility states,
A frontend task, no longer tame.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
Thank you for submitting this pull request! The changes introduce the ability to run the page scanner directly from a loaded frontend page by exposing a function on the window object, which aligns with the PR's objective. The addition of helper functions like isIframeContext and getIframeOptions improves code organization. The handling of potential null results from document.querySelector is a good correctness improvement.
Summary of Findings
- Missing Tests: The PR description indicates that tests covering the changes are missing. Adding comprehensive tests for the new
window.runAccessibilityScanfunction and the conditional auto-run logic is crucial to ensure correctness and prevent regressions. - Inconsistent Error Reporting in
onDone: The error message dispatched whenaxe.cleanup()fails depends on the initial scan result, which seems inconsistent. The cleanup failure error should likely be reported consistently. - Error Handling in
window.runAccessibilityScan: The.catchblock calls theonCompletecallback and then re-throws the error, resulting in dual error reporting (callback and promise rejection). Clarification on the intended behavior or adjustment to a more standard pattern might be beneficial. - Global Variable Declaration: The
tooLongTimeoutvariable is declared globally, although its usage is limited to the iframe context logic. While not a critical issue, limiting variable scope where possible can improve maintainability. (Note: This is a low severity issue and was not commented on directly as per review settings).
Merge Readiness
The changes implement the core functionality described in the PR. However, the checklist in the PR description indicates that tests are missing, which is a high-severity issue for new functionality and a public API. Additionally, there are medium-severity concerns regarding error handling consistency and patterns. I recommend addressing the missing tests and clarifying/adjusting the error handling before merging. I am unable to approve this pull request; please have other reviewers review and approve this code before merging.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/pageScanner/index.js (4)
22-29: Consider using optional chaining for more concise codeThe function is well-documented and logically sound, but could benefit from optional chaining as suggested by the static analysis tool.
-function isIframeContext() { - return !! ( body && body.hasAttribute( 'data-iframe-id' ) && body.hasAttribute( 'data-iframe-event-name' ) ); -} +function isIframeContext() { + return !!body?.hasAttribute( 'data-iframe-id' ) && body?.hasAttribute( 'data-iframe-event-name' ); +}🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
31-44: Fix typo in JSDoc commentSmall typo in the JSDoc comment.
-/** - * Get the iframe options from the body attributes/ - * - * @return {Object} {{configOptions: {}, runOptions: {}, iframeId: string | Attribute, eventName: string | Attribute, postId: string | Attribute}} - */ +/** + * Get the iframe options from the body attributes. + * + * @return {Object} {{configOptions: {}, runOptions: {}, iframeId: string | Attribute, eventName: string | Attribute, postId: string | Attribute}} + */
201-222: Well-structured global method for manual scan triggeringThe exposed
window.runAccessibilityScanfunction is well-documented, handles errors properly, and supports both callback and Promise patterns, giving flexibility to callers.However, consider adding parameter validation to ensure options has the expected structure:
window.runAccessibilityScan = async function( options = {} ) { + // Validate options + options.configOptions = options.configOptions || {}; + options.runOptions = options.runOptions || {}; + return scan( options ) .then( ( result ) => { if ( typeof options.onComplete === 'function' ) { options.onComplete( result ); } return result; } ) .catch( ( err ) => { if ( typeof options.onComplete === 'function' ) { options.onComplete( null, err ); } throw err; } ); };
224-235: Improved initialization logic with context-aware executionThe conditional initialization is a great improvement, making the code more versatile by automatically running in iframe contexts while enabling manual execution in other contexts. This fulfills the PR's objective of making the scan runnable from a frontend loaded page.
Consider adding a configurable timeout option when manually triggering scans:
window.runAccessibilityScan = async function( options = {} ) { + // Set up timeout if specified + if (options.timeoutInSeconds) { + clearTimeout(tooLongTimeout); + tooLongTimeout = setTimeout(() => { + if (typeof options.onTimeout === 'function') { + options.onTimeout('Scan timed out'); + } + }, options.timeoutInSeconds * 1000); + } + return scan( options ) .then( ( result ) => { + clearTimeout(tooLongTimeout); if ( typeof options.onComplete === 'function' ) { options.onComplete( result ); } return result; } ) .catch( ( err ) => { + clearTimeout(tooLongTimeout); if ( typeof options.onComplete === 'function' ) { options.onComplete( null, err ); } throw err; } ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/pageScanner/index.js(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pageScanner/index.js
[error] 28-28: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
src/pageScanner/index.js (6)
13-14: Good improvement: Global timeout variable for better scan managementMoving the timeout handling to a global variable allows for proper cleanup in the
onDonefunction, improving resource management.
88-92: Good defensive programming: Safe DOM element accessAdding a null check before accessing
outerHTMLprevents potential runtime errors when elements aren't found.
102-106: Good defensive programming: Safe DOM element access for incomplete itemsSimilar to the previous comment, this check protects against null reference errors.
185-185: Improved error handling clarity with empty string for success caseUsing an empty string to indicate no error is clearer than using a falsy value.
191-191: Enhanced error reporting with specific failure codesUsing descriptive error codes ('cleanup-failed') improves debugging and error handling.
197-197: Better error identification with error code constantsUsing a specific error code 'cleanup-not-exists' makes error handling more robust.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/pageScanner/index.js (1)
175-175:⚠️ Potential issueRemove unused parameter.
The
errorparameter is declared but never used in the function body, causing a linting error.-const onDone = ( violations = [], errorMsgs = [], error = false ) => { +const onDone = ( violations = [], errorMsgs = [] ) => {🧰 Tools
🪛 GitHub Actions: Lint JS
[error] 175-175: ESLint: 'error' is assigned a value but never used. (no-unused-vars)
♻️ Duplicate comments (1)
src/pageScanner/index.js (1)
208-222: Address dual error reporting pattern.This implementation still has the dual error reporting issue mentioned in previous reviews - errors are reported both via callback AND promise rejection.
Consider handling errors exclusively through the callback when provided:
window.runAccessibilityScan = async function( options = {} ) { return scan( options ) .then( ( result ) => { if ( typeof options.onComplete === 'function' ) { options.onComplete( result ); } return result; } ) .catch( ( err ) => { if ( typeof options.onComplete === 'function' ) { options.onComplete( null, err ); + return undefined; // Don't reject promise if callback handled the error } throw err; } ); };
🧹 Nitpick comments (2)
src/pageScanner/index.js (2)
22-29: Simplify using optional chaining as suggested by static analysis.The current implementation works but can be made more concise and safer using optional chaining.
-function isIframeContext() { - return !! ( body && body.hasAttribute( 'data-iframe-id' ) && body.hasAttribute( 'data-iframe-event-name' ) ); -} +function isIframeContext() { + return !! ( body?.hasAttribute( 'data-iframe-id' ) && body?.hasAttribute( 'data-iframe-event-name' ) ); +}🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
31-44: Add null safety check for body element.The function assumes
bodyexists but should handle the case where it might be null.function getIframeOptions() { + if ( ! body ) { + return { + configOptions: {}, + runOptions: {}, + iframeId: null, + eventName: null, + postId: null, + }; + } return { configOptions: {}, runOptions: {}, iframeId: body.getAttribute( 'data-iframe-id' ), eventName: body.getAttribute( 'data-iframe-event-name' ), postId: body.getAttribute( 'data-iframe-post-id' ), }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pageScanner/index.js(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pageScanner/index.js
[error] 28-28: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: Lint JS
src/pageScanner/index.js
[error] 175-175: ESLint: 'error' is assigned a value but never used. (no-unused-vars)
[error] 191-191: ESLint: Expected indentation of 4 tabs but found 0. (indent)
🔇 Additional comments (4)
src/pageScanner/index.js (4)
13-14: LGTM! Good improvement for timeout management.Adding the global
tooLongTimeoutvariable allows proper cleanup of timeouts and better control over long-running scans.
88-91: Excellent defensive programming!Adding null checks before accessing
outerHTMLprevents potential runtime errors when elements don't exist in the DOM.Also applies to: 102-105
185-198: Good improvement to error handling consistency.Changing from boolean to string error codes provides more specific error information and addresses the previous review feedback about inconsistent error handling.
🧰 Tools
🪛 GitHub Actions: Lint JS
[error] 191-191: ESLint: Expected indentation of 4 tabs but found 0. (indent)
224-235: Well-structured conditional auto-run logic.The new pattern properly separates manual scan triggering from automatic iframe scanning, improving modularity and allowing the scanner to be used in different contexts.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pageScanner/index.js (1)
202-223: Global scan method successfully implements the PR objective.The
window.runAccessibilityScanmethod effectively exposes the scan functionality globally, enabling manual scan triggering from the frontend as intended by the PR objective. The implementation is well-documented and properly handles both success and error cases.
🧹 Nitpick comments (2)
src/pageScanner/index.js (2)
22-29: Consider using optional chaining for cleaner code.The function logic is correct, but you can simplify it using optional chaining as suggested by the static analysis tool.
Apply this diff to use optional chaining:
function isIframeContext() { - return !! ( body && body.hasAttribute( 'data-iframe-id' ) && body.hasAttribute( 'data-iframe-event-name' ) ); + return !! ( body?.hasAttribute( 'data-iframe-id' ) && body?.hasAttribute( 'data-iframe-event-name' ) ); }🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
31-44: Fix typo in JSDoc comment.There's a minor typo in the JSDoc comment that should be corrected.
Apply this diff to fix the typo:
/** - * Get the iframe options from the body attributes/ + * Get the iframe options from the body attributes. * * @return {Object} {{configOptions: {}, runOptions: {}, iframeId: string | Attribute, eventName: string | Attribute, postId: string | Attribute}} */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pageScanner/index.js(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pageScanner/index.js
[error] 28-28: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
src/pageScanner/index.js (4)
13-14: Good timeout management improvement.The global timeout variable allows for proper cleanup and centralized timeout handling across the module.
88-91: Excellent defensive programming improvement.The null checks before accessing
outerHTMLprevent potential runtime errors whenquerySelectorreturns null. This is a solid defensive programming practice.Also applies to: 102-105
186-186: Improved error handling with descriptive error codes.The change from boolean to string error codes provides better error categorization and more descriptive error reporting. This is a good improvement for debugging and error handling.
Also applies to: 192-192, 198-198
225-236: Well-implemented conditional scan execution.The conditional execution based on iframe context is a smart architectural change that enables both automatic scanning (in iframe) and manual triggering (via global method). The timeout handling and error management are properly implemented.
This change allows the frontend highlighter app to access the scanner bundle for enhanced functionality.
Refactor the scanning logic to improve readability and maintainability. Introduce helper methods for kicking off the scan, running the scan, and saving results. Enhance error handling and user feedback during the scanning process.
|
The POT file has changed. Please update the POT file in your PR to keep translations up to date. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/frontendHighlighterApp/index.js (1)
941-966: Remove unnecessaryselfaliasing and use optional chaining.Arrow functions inherit
thisfrom their enclosing scope, so theselfalias is unnecessary. Also, use optional chaining for cleaner code.runAccessibilityScanAndSave( densityMetrics ) { - const self = this; const summary = document.querySelector( '.edac-highlight-panel-controls-summary' ); if ( summary ) { summary.textContent = __( 'Scanning...', 'accessibility-checker' ); summary.classList.remove( 'edac-error' ); } window.runAccessibilityScan().then( ( result ) => { - const postId = window.edacFrontendHighlighterApp && window.edacFrontendHighlighterApp.postID; - const nonce = window.edacFrontendHighlighterApp && window.edacFrontendHighlighterApp.restNonce; + const postId = window.edacFrontendHighlighterApp?.postID; + const nonce = window.edacFrontendHighlighterApp?.restNonce; if ( ! postId || ! nonce ) { - self.showWait( false ); - self.showScanError( 'Missing postId or nonce.' ); + this.showWait( false ); + this.showScanError( 'Missing postId or nonce.' ); return; } if ( ! result || ! result.violations || result.violations.length === 0 ) { - self.showWait( false ); - self.showScanError( 'No violations found, skipping save.' ); + this.showWait( false ); + this.showScanError( 'No violations found, skipping save.' ); return; } - self.saveScanResults( postId, nonce, result.violations, densityMetrics ); + this.saveScanResults( postId, nonce, result.violations, densityMetrics ); } ).catch( () => { - self.showWait( false ); - self.showScanError( 'Accessibility scan error.' ); + this.showWait( false ); + this.showScanError( 'Accessibility scan error.' ); } ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/classes/class-enqueue-frontend.php(1 hunks)src/frontendHighlighterApp/index.js(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/frontendHighlighterApp/index.js (1)
Learnt from: SteveJonesDev
PR: equalizedigital/accessibility-checker#880
File: src/pageScanner/rules/video-present.js:3-3
Timestamp: 2025-04-08T21:36:48.803Z
Learning: In the accessibility-checker, the broad selector pattern in video-present.js (`'video, iframe, object, source, [src], [class], [role]'`) is intentional, as it allows the JavaScript evaluation function in video-detected.js to perform case-insensitive matching and sophisticated pattern detection that CSS selectors cannot replicate.
🪛 PHPStan (2.1.15)
includes/classes/class-enqueue-frontend.php
107-107: Constant EDAC_PLUGIN_URL not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(constant.notFound)
🪛 Biome (1.9.4)
src/frontendHighlighterApp/index.js
[error] 949-950: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 950-951: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 986-986: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 942-943: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 969-970: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: make-pot
🔇 Additional comments (2)
includes/classes/class-enqueue-frontend.php (1)
110-110: LGTM!The addition of
scannerBundleUrlproperly provides the URL to the page scanner bundle using the correct WordPress function and constant.src/frontendHighlighterApp/index.js (1)
544-549: Good error handling implementation!Nice job replacing the TODO with actual error handling that provides user feedback.
|
The POT file has changed. Please update the POT file in your PR to keep translations up to date. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/classes/class-enqueue-frontend.php(1 hunks)
🧰 Additional context used
🪛 PHPStan (2.1.15)
includes/classes/class-enqueue-frontend.php
107-107: Constant EDAC_PLUGIN_URL not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(constant.notFound)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: make-pot
|
The POT file has changed. Please update the POT file in your PR to keep translations up to date. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/frontendHighlighterApp/index.js (3)
188-199: Initialize the_scanAttemptedflag and use safer property access.This duplicates a previous review comment. The scan retry logic needs the
_scanAttemptedflag initialized in the constructor, and the nested property access should use safer checks.Add this to the constructor (around line 21):
this.settings = { ...defaultSettings, ...settings }; +this._scanAttempted = false;And use safer property access:
-} else if ( ! self._scanAttempted && response.data && response.data[ 0 ] && response.data[ 0 ].code === -3 ) { +} else if ( ! self._scanAttempted && response.data?.[0]?.code === -3 ) {
906-932: Remove hardcoded fallback URL and improve error handling.This duplicates a previous review comment. The hardcoded fallback URL could cause issues in different WordPress installations.
-script.src = window.edacFrontendHighlighterApp?.scannerBundleUrl || '/wp-content/plugins/accessibility-checker/build/pageScanner.bundle.js'; +script.src = window.edacFrontendHighlighterApp?.scannerBundleUrl; +if (!script.src) { + self.showScanError('Scanner URL not available.'); + return; +}
970-998: Use WordPress REST API base URL and remove unnecessary aliasing.This duplicates a previous review comment. The hardcoded REST endpoint could break with non-standard WordPress configurations, and the unnecessary aliasing should be removed.
saveScanResults( postId, nonce, violations, densityMetrics ) { - const self = this; - fetch( '/wp-json/accessibility-checker/v1/post-scan-results/' + postId, { + const restUrl = window.edacFrontendHighlighterApp?.edacUrl + '/wp-json/accessibility-checker/v1/post-scan-results/' + postId; + fetch( restUrl, { method: 'POST', headers: { 'Content-Type': 'application/json', 'X-WP-Nonce': nonce, }, body: JSON.stringify( { violations, isSkipped: false, isFailure: false, densityMetrics, } ), } ) .then( ( response ) => response.json() ) .then( ( data ) => { - self.showWait( false ); - if ( data && data.success ) { + this.showWait( false ); + if ( data?.success ) { // Optionally show a success message or update UI } else { - self.showScanError( __( 'Saving failed.', 'accessibility-checker' ) ); + this.showScanError( __( 'Saving failed.', 'accessibility-checker' ) ); } } ) .catch( () => { - self.showWait( false ); - self.showScanError( __( 'Error saving scan results.', 'accessibility-checker' ) ); + this.showWait( false ); + this.showScanError( __( 'Error saving scan results.', 'accessibility-checker' ) ); } ); }
🧹 Nitpick comments (1)
src/frontendHighlighterApp/index.js (1)
943-968: Remove unnecessarythisaliasing and use optional chaining.The method has unnecessary aliasing and could benefit from optional chaining for safer property access.
-runAccessibilityScanAndSave( densityMetrics ) { - const self = this; - const summary = document.querySelector( '.edac-highlight-panel-controls-summary' ); - if ( summary ) { - summary.textContent = __( 'Scanning...', 'accessibility-checker' ); - summary.classList.remove( 'edac-error' ); - } - window.runAccessibilityScan().then( ( result ) => { - const postId = window.edacFrontendHighlighterApp && window.edacFrontendHighlighterApp.postID; - const nonce = window.edacFrontendHighlighterApp && window.edacFrontendHighlighterApp.restNonce; - if ( ! postId || ! nonce ) { - self.showWait( false ); - self.showScanError( __( 'Missing postId or nonce.', 'accessibility-checker' ) ); - return; - } - if ( ! result || ! result.violations || result.violations.length === 0 ) { - self.showWait( false ); - self.showScanError( __( 'No violations found, skipping save.', 'accessibility-checker' ) ); - return; - } - self.saveScanResults( postId, nonce, result.violations, densityMetrics ); - } ).catch( () => { - self.showWait( false ); - self.showScanError( __( 'Accessibility scan error.', 'accessibility-checker' ) ); - } ); -} +runAccessibilityScanAndSave( densityMetrics ) { + const summary = document.querySelector( '.edac-highlight-panel-controls-summary' ); + if ( summary ) { + summary.textContent = __( 'Scanning...', 'accessibility-checker' ); + summary.classList.remove( 'edac-error' ); + } + window.runAccessibilityScan().then( ( result ) => { + const postId = window.edacFrontendHighlighterApp?.postID; + const nonce = window.edacFrontendHighlighterApp?.restNonce; + if ( ! postId || ! nonce ) { + this.showWait( false ); + this.showScanError( __( 'Missing postId or nonce.', 'accessibility-checker' ) ); + return; + } + if ( ! result?.violations?.length ) { + this.showWait( false ); + this.showScanError( __( 'No violations found, skipping save.', 'accessibility-checker' ) ); + return; + } + this.saveScanResults( postId, nonce, result.violations, densityMetrics ); + } ).catch( () => { + this.showWait( false ); + this.showScanError( __( 'Accessibility scan error.', 'accessibility-checker' ) ); + } ); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/frontendHighlighterApp/index.js(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/frontendHighlighterApp/index.js (1)
Learnt from: SteveJonesDev
PR: equalizedigital/accessibility-checker#880
File: src/pageScanner/rules/video-present.js:3-3
Timestamp: 2025-04-08T21:36:48.803Z
Learning: In the accessibility-checker, the broad selector pattern in video-present.js (`'video, iframe, object, source, [src], [class], [role]'`) is intentional, as it allows the JavaScript evaluation function in video-detected.js to perform case-insensitive matching and sophisticated pattern detection that CSS selectors cannot replicate.
🪛 Biome (1.9.4)
src/frontendHighlighterApp/index.js
[error] 951-952: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 952-953: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 988-988: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 944-945: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 971-972: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ 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: make-pot
🔇 Additional comments (3)
src/frontendHighlighterApp/index.js (3)
544-548: Good error handling implementation.The error handling logic properly updates the UI summary with a user-friendly message when issues fail to load.
934-941: Clean implementation of scan validation.The method correctly checks for the global scanner function and provides appropriate error handling.
1000-1010: Well-implemented error display method.The method provides a clean fallback mechanism for showing scan errors in the UI with proper styling.
|
The POT file has changed. Please update the POT file in your PR to keep translations up to date. |
SteveJonesDev
left a comment
SteveJonesDev
left a comment
There was a problem hiding this comment.
Tested locally and appears to work as expected and doesn't seem to interfere with previous iFrame scans. Nice cleanup of TODOS and console logs.
2 participants
This PR adjusts the pageScanner bundle so that it can be invoked from the window object directly meaning it can be run from a loaded page on the frontend if a rescan is needed there.
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
Fixes: #1039