Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a proof-of-concept plugin system to make additional accessibility scans and tests modular. The implementation allows the scanner to load and execute custom test plugins alongside the existing axe-core scanner, with built-in and custom plugin support.
Changes:
- Added plugin architecture with dynamic loading from
.github/scanner-plugins/directories - Introduced configurable
scansinput to control which scanners/plugins to execute - Implemented a reflow-test plugin as a proof-of-concept to check for horizontal scrolling at 320x256 viewport
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| action.yml | Adds shell script to copy scanner-plugins directory to action workspace |
| .gitignore | Adds .vscode to ignored files |
| .github/scanner-plugins/reflow-test/package.json | Package metadata for the reflow test plugin |
| .github/scanner-plugins/reflow-test/index.js | Implementation of the reflow test plugin that checks viewport scrolling |
| .github/actions/find/tests/pluginManager.test.ts | Unit tests for plugin loading and caching functionality |
| .github/actions/find/tests/findForUrl.test.ts | Unit tests for the updated findForUrl with plugin support |
| .github/actions/find/src/scansContextProvider.ts | Provides context for determining which scans to perform based on input |
| .github/actions/find/src/pluginManager.ts | Core plugin loading logic for built-in and custom plugins |
| .github/actions/find/src/findForUrl.ts | Updated to support plugin execution alongside axe scans |
| .github/actions/find/src/dynamicImport.ts | Wrapper for dynamic imports to enable mocking in tests |
| .github/actions/find/action.yml | Adds new scans input parameter and formatting changes |
| .github/actions/find/README.md | Documents the new scans input parameter |
Comments suppressed due to low confidence (4)
.github/actions/find/src/pluginManager.ts:63
- Using string concatenation with
process.cwd() + '/.github/scanner-plugins/'is less robust than usingpath.join(). The current approach could cause issues with path separators on different operating systems. Consider usingpath.join(process.cwd(), '.github', 'scanner-plugins')for consistency and cross-platform compatibility, similar to how line 54 handles built-in plugins.
const pluginsPath = process.cwd() + '/.github/scanner-plugins/'
.github/actions/find/src/scansContextProvider.ts:23
- Spelling error: "compatability" should be "compatibility".
// - if no 'scans' input is provided, we default to the existing behavior
// (only axe scan) for backwards compatability.
.github/actions/find/src/pluginManager.ts:34
- There's a potential race condition in the plugin loading logic. If
loadPlugins()is called concurrently from multiple places before the first call completes, both calls will seepluginsLoadedas false and both will attempt to load plugins simultaneously, potentially causing duplicate plugins to be added to the array. While this may not be an issue in the current single-threaded JavaScript execution model, consider adding a loading state or using a promise-based lock to prevent concurrent loading attempts, especially if this code might be used in parallel contexts in the future.
export async function loadPlugins() {
console.log('loading plugins')
try {
if (!pluginsLoaded) {
await loadBuiltInPlugins()
await loadCustomPlugins()
}
} catch {
plugins.length = 0
console.log(abortError)
} finally {
pluginsLoaded = true
return plugins
}
.github/actions/find/src/scansContextProvider.ts:15
- Grammar issue: "its" should be "it's" (contraction of "it is").
// or we do have a scans input, but it only has 1 item and its 'axe'
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fi | ||
|
|
||
| mkdir -p "${ACTION_DIR}/.github/scanner-plugins" | ||
| if [ "$(realpath ".github/scanner-plugins")" != "$(realpath "${ACTION_DIR}/.github/scanner-plugins")" ]; then |
There was a problem hiding this comment.
The shell script doesn't check if the .github/scanner-plugins directory exists in the source before attempting to copy. If the directory doesn't exist (e.g., in a workflow repo that doesn't define custom plugins), the cp -a command will fail. Consider adding an existence check before the copy operation, similar to how it's done for the .github/actions directory on line 54. For example: if [ -d ".github/scanner-plugins" ] && [ "$(realpath ".github/scanner-plugins")" != "$(realpath "${ACTION_DIR}/.github/scanner-plugins")" ]; then
| if [ "$(realpath ".github/scanner-plugins")" != "$(realpath "${ACTION_DIR}/.github/scanner-plugins")" ]; then | |
| if [ -d ".github/scanner-plugins" ] && [ "$(realpath ".github/scanner-plugins")" != "$(realpath "${ACTION_DIR}/.github/scanner-plugins")" ]; then |
- update plugin manager tests to account for new isDirectory check
lindseywild
left a comment
There was a problem hiding this comment.
A few additional comments to what Joyce has said! Also, I am assuming we want to update the README with this info, but wait until it's released. Could we create a follow-up issue to update the README / instructions once this is released? I am not sure if you all had already chatted about a different process though.
- update console.logs to core.infos
This is a very minimal proof of concept around the idea of using plugins to make additional scans and tests modular. Looking for feedback on general approach, pros/cons, etc...
this currently works with the plugins defined in the scanner repo. I'm still working on making this work with plugins defined in the workflow repo (the repo that uses the action). There might be some issues with using absolute paths with the dynamic import() feature - but i'm still testing.
I've taken @lindseywild’s reflow test and added it as a plugin for testing. the idea is each plugin lives under a new folder under the scanner-plugins folder (which lives under .github like actions). and each plugin has a primary index.js file, which we will use to interface with the plugin.
all of this is just how I've built it as a proposal - we can tweak naming/folder-structure, etc... if we feel something else makes more sense