VACMS-24047: Moderate alerts root#24194
Conversation
|
Checking composer.lock changes... |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the JavaScript linting toolchain/configuration (likely to address moderate security alerts) by migrating to ESLint 9’s flat config and adjusting CI/scripts accordingly.
Changes:
- Upgrade ESLint/Prettier (and cypress-parallel) and add a new
eslint.config.jsflat configuration. - Update npm scripts and the CI ESLint workflow flags to rely on the flat config.
- Remove multiple inline
eslint-disablecomments across Cypress tests and custom JS.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cypress/support/commands.js | Removes disable comments (but leaves whitespace and retains console.log). |
| tests/cypress/support/accessibility.js | Removes disable comments (but leaves whitespace). |
| tests/cypress/integration/step_definitions/common/the_derivative_of_the_media_image_should_match_the_fixture.js | Removes disable comment at file header. |
| tests/cypress/integration/step_definitions/common/i_stub_form_submission.js | Removes disable comments near imports. |
| tests/cypress/integration/step_definitions/common/i_delete_the_derivative_of_the_media_image.js | Removes disable comment at file header. |
| tests/cypress/integration/step_definitions/common/i_create_a_node.js | Removes disable comment at file header. |
| tests/cypress/integration/step_definitions/api/vba_facility_api_data_flow.js | Removes disable comment near Chai assertion expression. |
| tests/cypress/integration/step_definitions/api/vamc_system_medical_records_office_api_data_flow.js | Removes disable comments near Chai assertion expressions. |
| tests/cypress/integration/step_definitions/api/listing_page_api_data_flow.js | Removes disable comment near Chai assertion expression. |
| package.json | Switches eslint scripts to default config, upgrades linting deps. |
| eslint.config.js | Adds new ESLint 9+ flat config with prettier/import plugins and globals. |
| docroot/modules/custom/va_gov_vamc/js/archive_confirm.es6.js | Re-formats early returns/focus call (currently not Prettier-formatted). |
| docroot/modules/custom/va_gov_media/js/google_analytics.es6.js | Re-formats early returns and catch blocks (currently not Prettier-formatted). |
| docroot/modules/custom/va_gov_media/js/alt_text_validation.es6.js | Removes no-undef disable comment (but leaves whitespace). |
| .github/workflows/continuous_integration.yml | Updates ESLint flags to rely on flat config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "max-nested-callbacks": ["warn", 3], | ||
| "import/no-mutable-exports": "warn", | ||
| "no-plusplus": ["warn", { allowForLoopAfterthoughts: true }], | ||
| "no-param-reassign": "off", | ||
| "no-prototype-builtins": "off", | ||
| // "valid-jsdoc": ["warn", { "prefer": { "returns": "return", "property": "prop" }, "requireReturn": false }], // Not supported in ESLint 9 | ||
| "no-unused-vars": "warn", | ||
| "operator-linebreak": ["error", "after", { "overrides": { "?": "ignore", ":": "ignore" } }], | ||
| // Best practices | ||
| "eqeqeq": ["error", "always", { null: "ignore" }], | ||
| "radix": "error", | ||
| "no-alert": "warn", | ||
| "no-console": "warn", |
There was a problem hiding this comment.
Several rules are set to "warn" while CI/scripts enforce --max-warnings 0, which will fail the lint step on any warning. Either change these to "error" (if they should block) or "off"/scoped overrides (if they are informational), or adjust the max-warnings policy—otherwise this configuration is self-contradictory and likely to break CI once warnings are encountered.
| "max-nested-callbacks": ["warn", 3], | |
| "import/no-mutable-exports": "warn", | |
| "no-plusplus": ["warn", { allowForLoopAfterthoughts: true }], | |
| "no-param-reassign": "off", | |
| "no-prototype-builtins": "off", | |
| // "valid-jsdoc": ["warn", { "prefer": { "returns": "return", "property": "prop" }, "requireReturn": false }], // Not supported in ESLint 9 | |
| "no-unused-vars": "warn", | |
| "operator-linebreak": ["error", "after", { "overrides": { "?": "ignore", ":": "ignore" } }], | |
| // Best practices | |
| "eqeqeq": ["error", "always", { null: "ignore" }], | |
| "radix": "error", | |
| "no-alert": "warn", | |
| "no-console": "warn", | |
| "max-nested-callbacks": ["error", 3], | |
| "import/no-mutable-exports": "error", | |
| "no-plusplus": ["error", { allowForLoopAfterthoughts: true }], | |
| "no-param-reassign": "off", | |
| "no-prototype-builtins": "off", | |
| // "valid-jsdoc": ["warn", { "prefer": { "returns": "return", "property": "prop" }, "requireReturn": false }], // Not supported in ESLint 9 | |
| "no-unused-vars": "error", | |
| "operator-linebreak": ["error", "after", { "overrides": { "?": "ignore", ":": "ignore" } }], | |
| // Best practices | |
| "eqeqeq": ["error", "always", { null: "ignore" }], | |
| "radix": "error", | |
| "no-alert": "error", | |
| "no-console": "error", |
| "eqeqeq": ["error", "always", { null: "ignore" }], | ||
| "radix": "error", | ||
| "no-alert": "warn", | ||
| "no-console": "warn", |
There was a problem hiding this comment.
Several rules are set to "warn" while CI/scripts enforce --max-warnings 0, which will fail the lint step on any warning. Either change these to "error" (if they should block) or "off"/scoped overrides (if they are informational), or adjust the max-warnings policy—otherwise this configuration is self-contradictory and likely to break CI once warnings are encountered.
| "no-console": "warn", | |
| "no-console": "error", |
| expect(uuid, `Expected to find a UUID for path: ${path}`).to.not.be | ||
| .undefined; |
There was a problem hiding this comment.
Removing the no-unused-expressions disable comment may reintroduce lint failures if no-unused-expressions (commonly enabled via ESLint recommended) is active, because Chai’s property assertions like .to.not.be.undefined; are treated as unused expressions. To avoid needing disables, prefer an assertion form that doesn’t rely on property side effects (e.g., to.exist, or to.not.equal(undefined)), or configure the rule appropriately for Chai/Cypress tests.
| expect(uuid, `Expected to find a UUID for path: ${path}`).to.not.be | |
| .undefined; | |
| expect(uuid, `Expected to find a UUID for path: ${path}`).to.not.equal( | |
| undefined | |
| ); |
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
|
Checking composer.lock changes... |
Description
Relates to #24047
Generated description
(Select this text, hit the Copilot button, and select "Generate".)
Testing done
Screenshots
QA steps
What needs to be checked to prove this works?
What needs to be checked to prove it didn't break any related things?
What variations of circumstances (users, actions, values) need to be checked?
As user uid with user_role
Definition of Done
Select Team for PR review
CMS TeamPublic websitesFacilitiesUser supportAccelerated PublishingIs this PR blocked by another PR?
DO NOT MERGEDoes this PR need review from a Product Owner
Needs PO reviewCMS user-facing announcement
Is an announcement needed to let editors know of this change?