Replaced Grunt with NPM scripts in demo theme. #2214
Conversation
WalkthroughReplaces the theme's Grunt-based build with an npm-script workflow, adds ESLint/PostCSS configs and README, updates theme JS and SCSS, adjusts git/docker ignore entries, and updates installer/tests to detect npm/postcss-based themes instead of requiring a Gruntfile. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Fix all issues with AI Agents 🤖
In @web/themes/custom/your_site_theme/.eslintrc.json:
- Line 35: The object property "bodyScrollLock" has an extra space before the
colon; update the property to remove the space so it matches the surrounding
properties' formatting (change `"bodyScrollLock" : true,` to `"bodyScrollLock":
true,`) ensuring consistent spacing and linting across the object.
In @web/themes/custom/your_site_theme/js/your_site_theme.js:
- Line 17: Remove the leftover development placeholder comment "give me example
code here that would be using context with body" from your_site_theme.js; locate
that exact comment in the file and delete it (or replace it with a meaningful
comment or real example code if needed) so no dev prompt remains in production
code.
In @web/themes/custom/your_site_theme/package.json:
- Line 40: The dev npm script "sass:dev" currently writes expanded CSS to
your_site_theme.min.css which is misleading; change the output filename in the
"sass:dev" script to a non-minified name (e.g., your_site_theme.css) and update
any related dev scripts like "postcss:dev" and any references in theme
build/deploy steps to use that non-minified name, or alternatively adjust the
dev script to use --style=compressed if you must keep your_site_theme.min.css.
- Line 54: The "watch" npm script currently runs chokidar 'js/**/*.js'
'scss/**/*.scss' -c 'npm run build-dev' and rebuilds on every change; update
that script to add chokidar's debounce and initial flags (e.g., --debounce 500
--initial) so the build waits ~500ms after the last change and runs once on
start, preventing multiple rapid builds; modify the package.json "watch" entry
accordingly so it still invokes npm run build-dev but with these chokidar
options.
- Line 45: The "uglify" npm script currently appends "|| true" which hides
terser failures (e.g., when build/js/your_site_theme.min.js is missing after
concat); remove the "|| true" so terser errors surface and CI/builds fail
visibly, and if you need graceful handling instead, add a pre-check for the
existence of build/js/your_site_theme.min.js (or the concat step) before
invoking terser so the script can either skip uglify cleanly or fail with a
clear error.
- Around line 37-38: The npm scripts copy:images and copy:fonts currently
swallow all errors via "2>/dev/null || true", which can hide real failures;
change each script to explicitly check for the source directory (images or
fonts) and: if the directory is missing emit a clear warning message, otherwise
create the target directory and run cp so that real errors (permissions, disk
space) surface; update the package.json entries "copy:images" and "copy:fonts"
accordingly (or replace them with a small Node/JS script used by those entries)
so missing-source cases log a warning while other failures still fail the
command.
- Line 44: The "concat" npm script currently only runs "cat js/**/*.js >
build/js/your_site_theme.min.js 2>/dev/null || mkdir -p build/js" which creates
the directory on failure but does not create the output file; update the
"concat" script so that on failure it also ensures the output file exists (for
example by running mkdir -p build/js and then touching
build/js/your_site_theme.min.js) or change the fallback to create the file
instead of only the folder so subsequent "uglify" steps won't fail; refer to the
"concat" script name and the target file build/js/your_site_theme.min.js when
making the change.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (50)
.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_all_circleci/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_all_gha/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_artifact/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/js/lightsaber.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_claro/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_claro/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/js/light_saber.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_olivero/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_olivero/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_stark/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_stark/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**web/themes/custom/your_site_theme/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.gitignore.artifactweb/themes/custom/your_site_theme/.eslintrc.jsonweb/themes/custom/your_site_theme/Gruntfile.jsweb/themes/custom/your_site_theme/README.mdweb/themes/custom/your_site_theme/js/your_site_theme.jsweb/themes/custom/your_site_theme/package.jsonweb/themes/custom/your_site_theme/postcss.config.js
💤 Files with no reviewable changes (1)
- web/themes/custom/your_site_theme/Gruntfile.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-01T08:09:18.993Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1693
File: .vortex/installer/tests/Fixtures/install/hosting_acquia/docs/deployment.md:6-8
Timestamp: 2025-06-01T08:09:18.993Z
Learning: Do not review files in test fixture directories (e.g., files under paths containing `/tests/Fixtures/` or similar test fixture patterns). These files are used for testing purposes and should remain as-is.
Applied to files:
.gitignore.artifact
⏰ 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). (9)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.2)
🔇 Additional comments (6)
web/themes/custom/your_site_theme/README.md (1)
1-24: LGTM! Clear and concise documentation.The README provides all essential information for developers to get started with the theme, including requirements, installation steps, and available commands.
.gitignore.artifact (1)
26-34: LGTM! Appropriate artifact exclusions for production builds.The ignore patterns correctly exclude source files, development dependencies, and build configuration from deployment artifacts while keeping compiled assets.
web/themes/custom/your_site_theme/postcss.config.js (1)
1-5: No action needed. Autoprefixer is already declared inpackage.jsonas a dependency (version ^10.4.22).web/themes/custom/your_site_theme/.eslintrc.json (1)
2-17: All required ESLint dependencies are properly declared in package.json. No action needed.The configuration extends and plugins referenced in
.eslintrc.jsonare all present indevDependencies:
eslint-config-airbnb-base✓eslint-plugin-prettier&eslint-config-prettier✓eslint-plugin-yml✓eslint-plugin-jsdoc✓eslint-plugin-import(peer dependency) ✓web/themes/custom/your_site_theme/package.json (2)
52-53: Build pipeline structure looks good, pending concat fix.The build pipeline follows a logical sequence: clean → prepare directories → process JS → process CSS → copy assets. The use of
&&ensures the build fails fast if any step fails, which is appropriate for CI/CD.However, the build will be unreliable until the
concatscript issue (line 44) is resolved.
6-29: Consider supply-chain vulnerability in eslint-config-prettier and eslint-plugin-prettier.The devDependencies are generally secure and up-to-date. However, CVE-2025-54313 affected eslint-config-prettier and eslint-plugin-prettier with malicious postinstall scripts in specific versions. The versions specified (^10.1.8 and ^5.5.4) appear to be patched releases, but verify your package-lock.json or yarn.lock to ensure no compromised versions are installed. Caret ranges (^) enable automatic patch-level security updates within each major version.
| "CKEditor5": true, | ||
| "tabbable": true, | ||
| "transliterate": true, | ||
| "bodyScrollLock" : true, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fix inconsistent spacing in object property.
There's an extra space before the colon in the bodyScrollLock property, which is inconsistent with the formatting of other properties in this section.
🔎 Proposed fix
- "bodyScrollLock" : true,
+ "bodyScrollLock": true,📝 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.
| "bodyScrollLock" : true, | |
| "bodyScrollLock": true, |
🤖 Prompt for AI Agents
In @web/themes/custom/your_site_theme/.eslintrc.json around line 35, The object
property "bodyScrollLock" has an extra space before the colon; update the
property to remove the space so it matches the surrounding properties'
formatting (change `"bodyScrollLock" : true,` to `"bodyScrollLock": true,`)
ensuring consistent spacing and linting across the object.
| attach(context) { | ||
| // give me example code here that would be using context with body | ||
| $(context) | ||
| .find('body') | ||
| .once('your-site-theme') | ||
| .each(function iterateBody() { | ||
| // Example: Add a class to the body element. | ||
| $(this).addClass('your-site-theme-processed'); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Fix incorrect body element selection logic.
The current implementation has a critical logic error:
$(context).find('body')won't work correctly becausebodyis rarely a descendant of thecontextparameter passed to Drupal behaviors- The jQuery
.once()method is deprecated in Drupal 10+; use the standaloneonce()function instead - For body element manipulation, you should either use
$('body')directly or check if context is the document
🔎 Proposed fix for modern Drupal (10+)
attach(context) {
- $(context)
- .find('body')
- .once('your-site-theme')
- .each(function iterateBody() {
- // Example: Add a class to the body element.
- $(this).addClass('your-site-theme-processed');
- });
+ // Only process if context is the document/body or contains it
+ const bodyElements = once('your-site-theme', 'body', context);
+ bodyElements.forEach((element) => {
+ // Example: Add a class to the body element.
+ element.classList.add('your-site-theme-processed');
+ });
},Or if you need to maintain jQuery usage:
attach(context) {
- $(context)
- .find('body')
- .once('your-site-theme')
- .each(function iterateBody() {
- // Example: Add a class to the body element.
- $(this).addClass('your-site-theme-processed');
- });
+ const bodyElements = once('your-site-theme', 'body', context);
+ $(bodyElements).addClass('your-site-theme-processed');
},| "copy:images": "mkdir -p build/images && cp -r images/* build/images/ 2>/dev/null || true", | ||
| "copy:fonts": "mkdir -p build/fonts && cp -r fonts/* build/fonts/ 2>/dev/null || true", |
There was a problem hiding this comment.
Error suppression may hide real issues.
The 2>/dev/null || true pattern suppresses all errors, including permission issues or disk space problems. While this gracefully handles missing source directories, it could mask legitimate failures.
Consider logging a warning when directories are missing:
🔎 Alternative approach with better feedback
- "copy:images": "mkdir -p build/images && cp -r images/* build/images/ 2>/dev/null || true",
- "copy:fonts": "mkdir -p build/fonts && cp -r fonts/* build/fonts/ 2>/dev/null || true",
+ "copy:images": "mkdir -p build/images && (cp -r images/* build/images/ 2>/dev/null || echo 'No images to copy')",
+ "copy:fonts": "mkdir -p build/fonts && (cp -r fonts/* build/fonts/ 2>/dev/null || echo 'No fonts to copy')",📝 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.
| "copy:images": "mkdir -p build/images && cp -r images/* build/images/ 2>/dev/null || true", | |
| "copy:fonts": "mkdir -p build/fonts && cp -r fonts/* build/fonts/ 2>/dev/null || true", | |
| "copy:images": "mkdir -p build/images && (cp -r images/* build/images/ 2>/dev/null || echo 'No images to copy')", | |
| "copy:fonts": "mkdir -p build/fonts && (cp -r fonts/* build/fonts/ 2>/dev/null || echo 'No fonts to copy')", |
🤖 Prompt for AI Agents
In @web/themes/custom/your_site_theme/package.json around lines 37-38, The npm
scripts copy:images and copy:fonts currently swallow all errors via "2>/dev/null
|| true", which can hide real failures; change each script to explicitly check
for the source directory (images or fonts) and: if the directory is missing emit
a clear warning message, otherwise create the target directory and run cp so
that real errors (permissions, disk space) surface; update the package.json
entries "copy:images" and "copy:fonts" accordingly (or replace them with a small
Node/JS script used by those entries) so missing-source cases log a warning
while other failures still fail the command.
| "copy:images": "mkdir -p build/images && cp -r images/* build/images/ 2>/dev/null || true", | ||
| "copy:fonts": "mkdir -p build/fonts && cp -r fonts/* build/fonts/ 2>/dev/null || true", | ||
| "copy": "npm run copy:images && npm run copy:fonts", | ||
| "sass:dev": "sass scss/styles.scss build/css/your_site_theme.min.css --source-map --style=expanded", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider renaming dev output file for clarity.
The dev build outputs to your_site_theme.min.css but uses --style=expanded, so the file isn't actually minified. Consider using a non-minified filename for dev builds.
🔎 Alternative naming approach
- "sass:dev": "sass scss/styles.scss build/css/your_site_theme.min.css --source-map --style=expanded",
+ "sass:dev": "sass scss/styles.scss build/css/your_site_theme.css --source-map --style=expanded",Then update postcss:dev and any references to use the non-minified name in dev mode, or keep the current approach if Drupal theme configuration expects a single filename.
📝 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.
| "sass:dev": "sass scss/styles.scss build/css/your_site_theme.min.css --source-map --style=expanded", | |
| "sass:dev": "sass scss/styles.scss build/css/your_site_theme.css --source-map --style=expanded", |
🤖 Prompt for AI Agents
In @web/themes/custom/your_site_theme/package.json around line 40, The dev npm
script "sass:dev" currently writes expanded CSS to your_site_theme.min.css which
is misleading; change the output filename in the "sass:dev" script to a
non-minified name (e.g., your_site_theme.css) and update any related dev scripts
like "postcss:dev" and any references in theme build/deploy steps to use that
non-minified name, or alternatively adjust the dev script to use
--style=compressed if you must keep your_site_theme.min.css.
| "postcss:dev": "postcss build/css/your_site_theme.min.css -o build/css/your_site_theme.min.css --map", | ||
| "postcss:prod": "postcss build/css/your_site_theme.min.css -o build/css/your_site_theme.min.css --no-map", | ||
| "concat": "cat js/**/*.js > build/js/your_site_theme.min.js 2>/dev/null || mkdir -p build/js", | ||
| "uglify": "terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal'] 2>/dev/null || true", |
There was a problem hiding this comment.
Error suppression masks build failures.
The || true pattern suppresses all errors from terser, including cases where the input file doesn't exist (if concat failed). This makes debugging build issues difficult.
🔎 Recommended fix
Remove the error suppression so build failures are visible:
- "uglify": "terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal'] 2>/dev/null || true",
+ "uglify": "terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal']",If you need to handle the case where no JS files exist gracefully, check for the file first:
- "uglify": "terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal'] 2>/dev/null || true",
+ "uglify": "test -f build/js/your_site_theme.min.js && terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal'] || echo 'No JS to minify'",📝 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.
| "uglify": "terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal'] 2>/dev/null || true", | |
| "uglify": "terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal']", |
| "uglify": "terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal'] 2>/dev/null || true", | |
| "uglify": "test -f build/js/your_site_theme.min.js && terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal'] || echo 'No JS to minify'", |
🤖 Prompt for AI Agents
In @web/themes/custom/your_site_theme/package.json around line 45, The "uglify"
npm script currently appends "|| true" which hides terser failures (e.g., when
build/js/your_site_theme.min.js is missing after concat); remove the "|| true"
so terser errors surface and CI/builds fail visibly, and if you need graceful
handling instead, add a pre-check for the existence of
build/js/your_site_theme.min.js (or the concat step) before invoking terser so
the script can either skip uglify cleanly or fail with a clear error.
| "lint-fix": "npm run lint-js-fix && npm run lint-css-fix", | ||
| "build-dev": "npm run clean && mkdir -p build/js && npm run concat && npm run sass:dev && npm run postcss:dev && npm run copy", | ||
| "build": "npm run clean && mkdir -p build/js && npm run concat && npm run uglify && npm run sass:prod && npm run postcss:prod && npm run copy", | ||
| "watch": "chokidar 'js/**/*.js' 'scss/**/*.scss' -c 'npm run build-dev'", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding debouncing to watch command.
The watch script rebuilds on every file change, which could trigger multiple builds if several files are saved in quick succession (e.g., auto-save or mass formatting).
🔎 Add debouncing for better performance
- "watch": "chokidar 'js/**/*.js' 'scss/**/*.scss' -c 'npm run build-dev'",
+ "watch": "chokidar 'js/**/*.js' 'scss/**/*.scss' --initial --debounce 500 -c 'npm run build-dev'",The --debounce 500 flag waits 500ms after the last change before running the build. The --initial flag runs the build once when watch starts.
📝 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.
| "watch": "chokidar 'js/**/*.js' 'scss/**/*.scss' -c 'npm run build-dev'", | |
| "watch": "chokidar 'js/**/*.js' 'scss/**/*.scss' --initial --debounce 500 -c 'npm run build-dev'", |
🤖 Prompt for AI Agents
In @web/themes/custom/your_site_theme/package.json around line 54, The "watch"
npm script currently runs chokidar 'js/**/*.js' 'scss/**/*.scss' -c 'npm run
build-dev' and rebuilds on every change; update that script to add chokidar's
debounce and initial flags (e.g., --debounce 500 --initial) so the build waits
~500ms after the last change and runs once on start, preventing multiple rapid
builds; modify the package.json "watch" entry accordingly so it still invokes
npm run build-dev but with these chokidar options.
This comment has been minimized.
This comment has been minimized.
|
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2214 +/- ##
==========================================
- Coverage 76.81% 76.20% -0.61%
==========================================
Files 112 105 -7
Lines 5844 5683 -161
Branches 44 0 -44
==========================================
- Hits 4489 4331 -158
+ Misses 1355 1352 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/replace-grunt-npm-scripts
branch
from
59487ca to
38895fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @.vortex/tests/phpunit/Traits/SutTrait.php:
- Around line 347-349: Remove the duplicate assertion for
'scripts/vortex/login-container-registry.sh' — there are two identical
assertFileExists calls; keep only one and ensure the surrounding assertions (the
other assertFileExists for 'scripts/vortex/login.sh') remain unchanged; update
the test in SutTrait (the method containing the assertFileExists calls) so each
expected file is asserted exactly once.
In @web/themes/custom/your_site_theme/package.json:
- Around line 6-29: package-lock.json is missing for the theme (package.json
lists devDependencies), so run npm install --package-lock-only (or npm install)
in the theme directory to generate a package-lock.json, verify it contains
locked versions for devDependencies, and add/commit the generated
package-lock.json to the repo to enable npm audit and consistent installs.
♻️ Duplicate comments (8)
web/themes/custom/your_site_theme/.eslintrc.json (1)
35-35: Fix inconsistent spacing in object property.The
bodyScrollLockproperty has an extra space before the colon ("bodyScrollLock" : true), which is inconsistent with the formatting of other properties.🔎 Proposed fix
- "bodyScrollLock" : true, + "bodyScrollLock": true,web/themes/custom/your_site_theme/js/your_site_theme.js (2)
17-17: Remove the placeholder comment.The comment "give me example code here that would be using context with body" appears to be a development prompt that should be removed before merging.
🔎 Proposed fix
attach(context) { - // give me example code here that would be using context with body $(context)
18-24: Fix incorrect body element selection logic.The current implementation has critical issues:
Incorrect selector:
$(context).find('body')won't work correctly becausebodyis rarely a descendant of thecontextparameter passed to Drupal behaviors. Context is typically a DOM fragment or specific element, not a parent ofbody.Deprecated API: The jQuery
.once()method is deprecated in Drupal 10+. Use the standaloneonce()function instead.Wrong approach: For body element manipulation, check if context is the document or use
once()directly on the 'body' selector.🔎 Proposed fix for modern Drupal (10+)
attach(context) { - // give me example code here that would be using context with body - $(context) - .find('body') - .once('your-site-theme') - .each(function iterateBody() { - // Example: Add a class to the body element. - $(this).addClass('your-site-theme-processed'); - }); + // Only process if context is the document/body or contains it + const bodyElements = once('your-site-theme', 'body', context); + bodyElements.forEach((element) => { + // Example: Add a class to the body element. + element.classList.add('your-site-theme-processed'); + }); },web/themes/custom/your_site_theme/package.json (5)
39-39: Fallback logic doesn't create the output file on concatenation failure.The concat script creates the directory on failure but not the output file itself, which will cause the uglify step to fail. A past review comment already identified this issue.
41-42: Error suppression may hide real failures.The
2>/dev/null || truepattern in copy:fonts and copy:images suppresses all errors, including legitimate failures like permission issues. A past review comment already identified this concern.
52-52: Dev build outputs to .min.css but uses expanded style.The sass:dev script outputs to your_site_theme.min.css but uses
--style=expanded, which is misleading. A past review comment already noted this naming inconsistency.
54-54: Error suppression masks terser failures.The
2>/dev/null || truepattern in the uglify script suppresses all terser errors, making it difficult to debug build issues. A past review comment already identified this as a major concern.
55-55: Consider adding debouncing to watch command.The watch script rebuilds on every file change without debouncing, which could trigger multiple rapid builds. A past review comment already suggested adding the
--debounceflag.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (78)
.vortex/installer/tests/Fixtures/handler_process/_baseline/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_all_circleci/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_all_gha/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_artifact/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/js/lightsaber.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/provision_database_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_claro/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_claro/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/js/light_saber.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_olivero/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_olivero/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_stark/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_stark/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint_circleci/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_tests/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_tests_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_behat/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_behat_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs_circleci/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpmd/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpmd_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpstan/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpstan_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpunit/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpunit_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_rector/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_rector_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_none/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_none/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**web/themes/custom/your_site_theme/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.dockerignore.gitignore.artifact.vortex/docs/content/drupal/theme-scaffold.mdx.vortex/installer/src/Prompts/Handlers/Theme.php.vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php.vortex/tests/phpunit/Traits/SutTrait.phpweb/themes/custom/your_site_theme/.eslintrc.jsonweb/themes/custom/your_site_theme/Gruntfile.jsweb/themes/custom/your_site_theme/README.mdweb/themes/custom/your_site_theme/js/your_site_theme.jsweb/themes/custom/your_site_theme/package.jsonweb/themes/custom/your_site_theme/postcss.config.js
💤 Files with no reviewable changes (3)
- .vortex/docs/content/drupal/theme-scaffold.mdx
- .vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php
- web/themes/custom/your_site_theme/Gruntfile.js
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-07-23T01:16:30.963Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as `required=true` in Docker build mounts. The conditional check `if [ -s /run/secrets/github_token ]` is the preferred approach to allow builds to proceed without a token when needed.
Applied to files:
web/themes/custom/your_site_theme/package.json
📚 Learning: 2025-06-01T08:09:18.993Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1693
File: .vortex/installer/tests/Fixtures/install/hosting_acquia/docs/deployment.md:6-8
Timestamp: 2025-06-01T08:09:18.993Z
Learning: Do not review files in test fixture directories (e.g., files under paths containing `/tests/Fixtures/` or similar test fixture patterns). These files are used for testing purposes and should remain as-is.
Applied to files:
.gitignore.artifact
📚 Learning: 2025-09-13T04:14:41.765Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2011
File: .vortex/tests/phpunit/Traits/Steps/StepAhoyTrait.php:20-23
Timestamp: 2025-09-13T04:14:41.765Z
Learning: In .vortex/tests/phpunit PHPUnit tests using cmd/cmdFail helpers, patterns in the expected output arrays support '*' markers for positive assertions (text must be present) and '!' markers for negative assertions (text must NOT be present). The '*' markers are NOT literal text matches but rather indicate expected presence.
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Use descriptive step definitions and discover available steps via 'ahoy test-bdd -- --definitions=l'
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Create supporting entities (taxonomies, media) in Behat Background section before content type tests to ensure test prerequisites exist
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Always prefix test content with [TEST] to distinguish test data from real content
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Test all visible fields in content type view scenarios to ensure proper display configuration
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Use numbered patterns for test content when creating multiple related test items: '[TEST] Topic 1', '[TEST] Topic 2'
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
⏰ 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). (11)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.3)
🔇 Additional comments (17)
.vortex/tests/phpunit/Traits/SutTrait.php (6)
251-268: LGTM!The new absence checks correctly verify that template placeholder files (YourSite test files) don't exist before installation. The organization into Functional/Kernel/Unit directories aligns with the test suite restructuring.
287-292: LGTM!New Docker configuration file assertions are appropriately placed with other Docker-related checks.
370-388: LGTM!Root configuration file assertions are properly updated to reflect the current project structure.
430-438: LGTM!The test file assertions properly reflect the restructured test organization into Functional/Kernel/Unit categories with appropriate naming conventions.
469-482: LGTM!Theme file assertions properly reflect the Grunt to NPM scripts migration:
postcss.config.jsreplaces Grunt configuration for CSS processing- Theme test base files follow the consistent Functional/Kernel/Unit structure
457-460: LGTM!Settings file assertions are properly updated.
.vortex/installer/src/Prompts/Handlers/Theme.php (1)
238-244: LGTM! Clean migration from Grunt to npm-script detection.The simplified detection logic correctly removes the Gruntfile.js requirement while preserving essential checks for Vortex theme characteristics. The method still validates build tooling presence via the "build-dev" string in package.json, making it compatible with the npm-script workflow.
Test expectations in
SubtestDeploymentTrait.php(line 94) already confirm the intended behavior—Gruntfile.js is correctly expected to not exist in deployments. The detection logic properly identifies Vortex themes during development (via package.json presence and "build-dev" script) before removing these development files during deployment..gitignore.artifact (2)
22-23: LGTM: Alphabetical ordering improves maintainability.The reordering of scaffold files improves readability without changing functionality.
26-34: LGTM: Correctly excludes npm tooling from artifacts.The addition of
.eslintrc.json,postcss.config.js, andscssto the ignore list properly ensures build-time assets are excluded from deployment artifacts, consistent with the migration to npm-based workflows..dockerignore (1)
32-54: LGTM: Whitelist properly updated for npm-based tooling.The additions of
.eslintrc.json,.prettierignore,.prettierrc.json,postcss.config.js,package.json,gherkinlint.json, andyarn.lockto the whitelist ensure that all necessary tooling files for the npm-based workflow are included in the Docker build context. The removal ofGruntfile.jsfrom the whitelist correctly reflects the migration away from Grunt.web/themes/custom/your_site_theme/.eslintrc.json (1)
1-72: LGTM: Comprehensive and well-structured ESLint configuration.The migration from the legacy Drupal ESLint config to a modern, explicit configuration is well-executed. The setup appropriately:
- Extends industry-standard style guides (airbnb-base) and integrates Prettier
- Defines all necessary Drupal globals (Drupal, jQuery, drupalSettings, etc.)
- Configures JSDoc validation rules suitable for Drupal theme development
- Balances strict linting with practical exceptions for Drupal patterns
web/themes/custom/your_site_theme/postcss.config.js (1)
1-5: No action required—browserslist is already configured.The
browserslistconfiguration is already present inpackage.jsonwith target browsers specified (last 2 versions,not ie <= 8,iOS >= 7). Autoprefixer will automatically use this configuration to determine which CSS prefixes to add.Likely an incorrect or invalid review comment.
web/themes/custom/your_site_theme/README.md (1)
1-24: LGTM! Clear and comprehensive documentation.The README provides all essential information for theme development: requirements, installation, and available commands. The documented commands align with the npm scripts in package.json.
web/themes/custom/your_site_theme/package.json (4)
36-38: LGTM! Build pipeline is well-structured.The build and build-dev scripts follow a logical sequence: clean, create directories, process JS/CSS, and copy assets. The development build appropriately skips minification for faster rebuilds.
43-48: LGTM! Comprehensive linting setup.The lint scripts provide both check and fix variants for JS and CSS, properly ignore minified files, and follow standard patterns.
49-50: LGTM! PostCSS configuration is appropriate.The dev and prod variants correctly handle source maps, with dev including them and prod omitting them for optimized output.
51-51: LGTM! Standard patch-package integration.The postinstall hook correctly applies package patches after installation.
This comment has been minimized.
This comment has been minimized.
|
|
|
AlexSkrypnyk
force-pushed
the
feature/replace-grunt-npm-scripts
branch
from
38895fb to
c49f226
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php:
- Around line 62-67: The tests currently use logSubstep and assertFileExists to
check assets were copied but do not validate contents; update
SubtestDockerComposeTrait's assertions after the existing assertFileExists calls
to also compare source vs build files using PHPUnit's assertFileEquals (or read
both with file_get_contents and use assertSame/assertEquals) for the image and
font paths (e.g., compare the original source files in
themes/custom/star_wars/src/images/test.svg and src/fonts/test.woff2 to the
build targets themes/custom/star_wars/build/images/test.svg and
build/fonts/test.woff2) so the tests assert both existence and identical
content.
In @web/themes/custom/your_site_theme/README.md:
- Around line 23-24: Update the README table so both commands consistently state
what they target: change the description for `yarn run lint-fix` to mirror `yarn
run lint` by appending "(JS and SCSS)" so it reads that it fixes code style
issues for JS and SCSS, matching the explicit scope used for `yarn run lint`.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/themes/custom/your_site_theme/scss/_rem.scss (1)
29-32: Critical: Fix invalid Sass function call.Line 32 uses
meta.append(), but the Sassmetamodule doesn't provide anappendfunction. This should belist.append()to match the usage elsewhere in the file (lines 30, 37, 39).This will cause Sass compilation to fail with the new npm build scripts.
🔎 Proposed fix
- } @else if meta.type-of($value) == "number" and math.unit($value) == "px" and $to == "rem" { - $result: meta.append($result, math.div($value, $rem-baseline) * 1rem, $separator); + } @else if meta.type-of($value) == "number" and math.unit($value) == "px" and $to == "rem" { + $result: list.append($result, math.div($value, $rem-baseline) * 1rem, $separator);
♻️ Duplicate comments (8)
web/themes/custom/your_site_theme/js/your_site_theme.js (2)
17-17: Remove the placeholder comment (previously flagged).This development prompt comment was flagged in a previous review and remains unaddressed. Please remove it before merging.
18-24: Fix the incorrect body element selection logic (previously flagged).This critical issue was identified in a previous review and remains unaddressed:
$(context).find('body')won't work correctly becausebodyis rarely a descendant of thecontextparameter- jQuery
.once()is deprecated in Drupal 10+; use the standaloneonce()function- Should use
$('body')directly or check if context is the documentPlease refer to the previous review comment for detailed proposed fixes using the modern
once()API..vortex/tests/phpunit/Traits/SutTrait.php (1)
347-349: Duplicate assertion detected.Line 347 and 348 both assert the existence of the same file
scripts/vortex/login-container-registry.sh. This appears to be a copy-paste error.🔎 Proposed fix to remove duplicate
$this->assertFileExists('scripts/vortex/login-container-registry.sh'); - $this->assertFileExists('scripts/vortex/login-container-registry.sh'); $this->assertFileExists('scripts/vortex/login.sh');web/themes/custom/your_site_theme/package.json (4)
6-29: Ensure package-lock.json exists for consistent installs.The devDependencies are current, but a lockfile is required for security auditing and reproducible builds.
41-42: Error suppression may hide real issues in copy commands.The
2>/dev/null || truepattern suppresses all errors, potentially masking permission or disk space issues.
54-54: Error suppression masks build failures in uglify.The
|| truepattern suppresses terser failures, making debugging difficult when the input file is missing.
55-55: Consider adding debouncing to watch command.The watch script rebuilds on every file change without debouncing, potentially triggering multiple rapid builds.
web/themes/custom/your_site_theme/.eslintrc.json (1)
35-35: Fix inconsistent spacing in object property.The
bodyScrollLockproperty has an extra space before the colon, inconsistent with other properties.🔎 Proposed fix
- "bodyScrollLock" : true, + "bodyScrollLock": true,
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (83)
.vortex/installer/tests/Fixtures/handler_process/_baseline/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_all_circleci/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_all_gha/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_artifact/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/js/lightsaber.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/provision_database_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_claro/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_claro/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/js/light_saber.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_olivero/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_olivero/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_stark/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_stark/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint_circleci/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_tests/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_tests_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_behat/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_behat_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs_circleci/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpmd/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpmd_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpstan/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpstan_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpunit/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpunit_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_rector/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_rector_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_none/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_none/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**web/themes/custom/your_site_theme/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
.dockerignore.gitignore.artifact.vortex/docs/content/drupal/theme-scaffold.mdx.vortex/installer/src/Prompts/Handlers/Theme.php.vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php.vortex/tests/phpunit/Traits/SutTrait.phpweb/themes/custom/your_site_theme/.eslintrc.jsonweb/themes/custom/your_site_theme/Gruntfile.jsweb/themes/custom/your_site_theme/README.mdweb/themes/custom/your_site_theme/js/your_site_theme.jsweb/themes/custom/your_site_theme/package.jsonweb/themes/custom/your_site_theme/postcss.config.jsweb/themes/custom/your_site_theme/scss/_rem.scss
💤 Files with no reviewable changes (3)
- .vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php
- web/themes/custom/your_site_theme/Gruntfile.js
- .vortex/docs/content/drupal/theme-scaffold.mdx
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-06-01T08:09:18.993Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1693
File: .vortex/installer/tests/Fixtures/install/hosting_acquia/docs/deployment.md:6-8
Timestamp: 2025-06-01T08:09:18.993Z
Learning: Do not review files in test fixture directories (e.g., files under paths containing `/tests/Fixtures/` or similar test fixture patterns). These files are used for testing purposes and should remain as-is.
Applied to files:
.gitignore.artifact
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-09-13T04:14:41.765Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2011
File: .vortex/tests/phpunit/Traits/Steps/StepAhoyTrait.php:20-23
Timestamp: 2025-09-13T04:14:41.765Z
Learning: In .vortex/tests/phpunit PHPUnit tests using cmd/cmdFail helpers, patterns in the expected output arrays support '*' markers for positive assertions (text must be present) and '!' markers for negative assertions (text must NOT be present). The '*' markers are NOT literal text matches but rather indicate expected presence.
Applied to files:
.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-06-02T00:41:36.321Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1697
File: .vortex/installer/tests/Fixtures/install/provision_database_lagoon/docs/onboarding.md:16-17
Timestamp: 2025-06-02T00:41:36.321Z
Learning: DO NOT EVER REVIEW FILES IN .vortex/installer/tests/Fixtures/ directory - these are test fixtures and should not be reviewed under any circumstances.
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-05-29T12:15:32.188Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in `.vortex/installer/tests/Fixtures/install` directory as they are test fixtures.
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Use descriptive step definitions and discover available steps via 'ahoy test-bdd -- --definitions=l'
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Create supporting entities (taxonomies, media) in Behat Background section before content type tests to ensure test prerequisites exist
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Always prefix test content with [TEST] to distinguish test data from real content
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Test all visible fields in content type view scenarios to ensure proper display configuration
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Use numbered patterns for test content when creating multiple related test items: '[TEST] Topic 1', '[TEST] Topic 2'
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-07-23T01:16:30.963Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as `required=true` in Docker build mounts. The conditional check `if [ -s /run/secrets/github_token ]` is the preferred approach to allow builds to proceed without a token when needed.
Applied to files:
web/themes/custom/your_site_theme/package.json
⏰ 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). (11)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-common
🔇 Additional comments (20)
web/themes/custom/your_site_theme/js/your_site_theme.js (2)
2-13: LGTM! Excellent JSDoc improvements.The updated documentation with proper
@file,@param, and@globaltags significantly improves code clarity and IDE support. The parameter type annotations for the IIFE wrapper are correctly specified.
14-14: LGTM! Named IIFE improves debuggability.Naming the IIFE
YourSiteThemeBehaviorsis a good practice that produces clearer stack traces and improves code maintainability..vortex/tests/phpunit/Traits/SutTrait.php (9)
251-256: LGTM!The assertions correctly verify that template test files are absent after the installer processes them. The pattern covers all three test types (Functional, Kernel, Unit) with both base classes and example tests.
257-257: LGTM!The assertions appropriately verify the absence of profile directories, example settings, workflow configurations, and documentation files before installation.
Also applies to: 261-261, 266-268
287-287: LGTM!Docker configuration file assertions are correct and align with the expected Vortex infrastructure files.
Also applies to: 292-292
336-336: LGTM!Script file assertions correctly verify the presence of core Vortex deployment and database download scripts.
Also applies to: 340-340, 342-343
354-356: LGTM!The assertions correctly verify the presence of script files, configuration examples, and documentation, as well as the appropriate absence of
.gitattributes.Also applies to: 370-370, 384-384, 388-388
430-430: LGTM!The assertions correctly verify the presence of the renamed module test files (SwBase*) after installation, which complements the earlier checks that verified the absence of the template files (YourSite*).
Also applies to: 434-438
457-457: LGTM!Settings file assertions correctly verify the presence of required Drupal configuration files after installation.
Also applies to: 459-460
469-471: LGTM!The assertions correctly verify the presence of NPM-based build tooling files (
package.json,postcss.config.js) and theme JavaScript, aligning with the PR's objective to replace Grunt with NPM scripts.
478-482: LGTM!The assertions correctly verify the presence of the theme test files (StarWars* base classes and example tests) after installation, maintaining consistency with the module test file pattern.
.dockerignore (1)
32-54: LGTM! Config files appropriately whitelisted for npm-based tooling.The whitelisted files (ESLint, Prettier, PostCSS configs, package.json, yarn.lock) are correctly included to support the new npm-script-driven workflow.
.gitignore.artifact (1)
22-34: LGTM! Artifact ignore patterns correctly updated for npm workflow.The theme asset ignores appropriately exclude source files (scss, node_modules) and tooling configs (ESLint, PostCSS, package.json) from deployment artifacts.
web/themes/custom/your_site_theme/package.json (2)
36-38: LGTM! Build scripts properly chain tasks with failure propagation.The build pipeline correctly ensures directories exist before file operations and uses
&&to stop on errors.
39-39: LGTM! Concat script improved with portable find command.The
findcommand properly excludes minified files and is more portable than shell glob expansion. The absence of fallback logic is appropriate since the build script ensuresbuild/jsexists before concat runs.web/themes/custom/your_site_theme/postcss.config.js (1)
1-5: LGTM! Standard PostCSS configuration with autoprefixer.The configuration correctly requires autoprefixer, which is properly declared in package.json. This is the standard minimal PostCSS setup.
web/themes/custom/your_site_theme/scss/_rem.scss (1)
43-47: LGTM!The conditional return logic correctly handles single-element versus multi-element results for the rem conversion function.
.vortex/installer/src/Prompts/Handlers/Theme.php (1)
238-244: The theme detection logic is correctly implemented and reliable for identifying Vortex themes.The
isVortexTheme()method successfully identifies Vortex themes through three key indicators: the presence ofscss/_variables.scss,package.json, and thebuild-devnpm script. The demo theme in the repository (your_site_theme) includes all three markers, confirming the detection logic works as expected.The method is properly used at line 169 to prevent accidental removal of custom Vortex theme files from the template during installation, and the combination of checks is sufficient to reliably distinguish Vortex themes from other custom themes.
.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php (2)
7-7: LGTM! New utility import for file operations.The
AlexSkrypnyk\File\Filedependency is appropriate for creating test fixtures on lines 25-26.
23-26: Consider adding explicit error handling for fixture file creation.The test creates dummy fixture files using the external
File::dump()library but doesn't verify creation succeeded. If fixture creation fails, the subsequent build assertions (lines 64 and 67) will report misleading errors ("file not copied") rather than the actual problem ("fixture not created").The fake wOFF2 content (
'wOFF2test') is intentional for testing—the build process only needs to copy files without validating their formats. However, clarify thatFile::dump()automatically creates parent directories (images/,fonts/) or add setup assertions if this isn't guaranteed.
| | `yarn run lint` | Check code style (JS and SCSS) | | ||
| | `yarn run lint-fix` | Fix code style issues automatically | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Improve consistency in command descriptions.
Line 23 explicitly mentions "(JS and SCSS)" for the lint command, but line 24's lint-fix description doesn't clarify that it fixes both JS and SCSS issues. Consider updating for consistency:
-| `yarn run lint-fix` | Fix code style issues automatically |
+| `yarn run lint-fix` | Fix code style issues automatically (JS and SCSS) |📝 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.
| | `yarn run lint` | Check code style (JS and SCSS) | | |
| | `yarn run lint-fix` | Fix code style issues automatically | | |
| | `yarn run lint` | Check code style (JS and SCSS) | | |
| | `yarn run lint-fix` | Fix code style issues automatically (JS and SCSS) | |
🤖 Prompt for AI Agents
In @web/themes/custom/your_site_theme/README.md around lines 23-24, Update the
README table so both commands consistently state what they target: change the
description for `yarn run lint-fix` to mirror `yarn run lint` by appending "(JS
and SCSS)" so it reads that it fixes code style issues for JS and SCSS, matching
the explicit scope used for `yarn run lint`.
This comment has been minimized.
This comment has been minimized.
|
|
|
AlexSkrypnyk
force-pushed
the
feature/replace-grunt-npm-scripts
branch
from
c49f226 to
2044c43
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @web/themes/custom/your_site_theme/js/your_site_theme.js:
- Around line 2-6: The file-level JSDoc block contains redundant @param tags for
$ and Drupal; remove those @param entries from the top-level block (leaving
@file and @global as appropriate) and keep the existing @param tags on the
actual function JSDoc (the function that takes $ and Drupal) so parameter docs
remain only on the function-level comment.
In @web/themes/custom/your_site_theme/postcss.config.js:
- Around line 1-5: Add CSS minification by updating the PostCSS plugins array in
postcss.config.js to include cssnano alongside the existing autoprefixer;
require('cssnano') should be added to the plugins list and invoked with the
preset 'default' (i.e., pass the config object to cssnano) so CSS output is
minified during the PostCSS step.
In @web/themes/custom/your_site_theme/scss/_rem.scss:
- Around line 43-47: No change required: the refactor expanding the ternary-like
conditional into an explicit @if/@else block that returns list.nth($result, 1)
when list.length($result) == 1 and returns $result otherwise is correct and
improves readability; keep the updated block as-is in _rem.scss (the @if
list.length($result) check and the two @return branches).
♻️ Duplicate comments (10)
.vortex/tests/phpunit/Traits/SutTrait.php (1)
347-349: Duplicate assertion still present.Lines 347 and 348 both assert the same file
scripts/vortex/login-container-registry.sh. This duplicate was flagged in a previous review and marked as addressed, but it still exists in the current code.🔎 Proposed fix to remove duplicate
$this->assertFileExists('scripts/vortex/login-container-registry.sh'); - $this->assertFileExists('scripts/vortex/login-container-registry.sh'); $this->assertFileExists('scripts/vortex/login.sh');web/themes/custom/your_site_theme/package.json (5)
6-29: Missing package-lock.json remains unaddressed.The previous review identified that a lockfile is required for security auditing and consistent installs. This issue persists - no
package-lock.jsonis included in the PR.
41-42: Error suppression in copy scripts remains.The previous review identified that
2>/dev/null || truesuppresses all errors, potentially masking permission or disk space issues. This concern remains unaddressed in the current implementation.
52-52: Dev build filename remains misleading.The previous review noted that
sass:devoutputs toyour_site_theme.min.cssdespite using--style=expanded. The.min.cssextension is misleading for non-minified output.
54-54: Error suppression in uglify script remains.The previous review identified that
|| truemasks terser failures, making debugging difficult. This issue persists in the current implementation.
55-55: Watch script lacks debouncing.The previous review suggested adding
--debounce 500 --initialto prevent multiple rapid builds when files are saved in quick succession. This enhancement has not been implemented.web/themes/custom/your_site_theme/js/your_site_theme.js (2)
16-25: Critical body selection issue remains unresolved.The previous review identified that
$(context).find('body')won't work correctly becausebodyis rarely a descendant of thecontextparameter, and that jQuery's.once()method is deprecated in Drupal 10+. This issue remains unaddressed in the current code.
17-17: Placeholder comment still present despite being marked as addressed.The development placeholder comment was previously flagged for removal and marked as addressed in commit c49f226, but it still appears in the current code.
web/themes/custom/your_site_theme/.eslintrc.json (1)
35-35: Inconsistent spacing remains unaddressed.The extra space before the colon in
"bodyScrollLock" : truewas previously flagged and remains in the current code.web/themes/custom/your_site_theme/README.md (1)
23-24: Command description consistency improvement suggested previously.The previous review noted that the
lintcommand explicitly mentions "(JS and SCSS)" whilelint-fixdoes not, suggesting adding the same clarification to line 24 for consistency.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (83)
.vortex/installer/tests/Fixtures/handler_process/_baseline/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_all_circleci/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_all_gha/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_artifact/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/js/lightsaber.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/provision_database_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_claro/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_claro/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/js/light_saber.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_olivero/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_olivero/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_stark/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_stark/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint_circleci/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_tests/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_tests_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_behat/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_behat_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs_circleci/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpmd/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpmd_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpstan/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpstan_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpunit/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpunit_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_rector/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_rector_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_none/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_none/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**web/themes/custom/your_site_theme/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
.dockerignore.gitignore.artifact.vortex/docs/content/drupal/theme-scaffold.mdx.vortex/installer/src/Prompts/Handlers/Theme.php.vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php.vortex/tests/phpunit/Traits/Subtests/SubtestDeploymentTrait.php.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php.vortex/tests/phpunit/Traits/SutTrait.phpweb/themes/custom/your_site_theme/.eslintrc.jsonweb/themes/custom/your_site_theme/Gruntfile.jsweb/themes/custom/your_site_theme/README.mdweb/themes/custom/your_site_theme/js/your_site_theme.jsweb/themes/custom/your_site_theme/package.jsonweb/themes/custom/your_site_theme/postcss.config.jsweb/themes/custom/your_site_theme/scss/_rem.scss
💤 Files with no reviewable changes (3)
- .vortex/docs/content/drupal/theme-scaffold.mdx
- web/themes/custom/your_site_theme/Gruntfile.js
- .vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-07-23T01:16:30.963Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as `required=true` in Docker build mounts. The conditional check `if [ -s /run/secrets/github_token ]` is the preferred approach to allow builds to proceed without a token when needed.
Applied to files:
web/themes/custom/your_site_theme/package.json
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-09-13T04:14:41.765Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2011
File: .vortex/tests/phpunit/Traits/Steps/StepAhoyTrait.php:20-23
Timestamp: 2025-09-13T04:14:41.765Z
Learning: In .vortex/tests/phpunit PHPUnit tests using cmd/cmdFail helpers, patterns in the expected output arrays support '*' markers for positive assertions (text must be present) and '!' markers for negative assertions (text must NOT be present). The '*' markers are NOT literal text matches but rather indicate expected presence.
Applied to files:
.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php.vortex/tests/phpunit/Traits/Subtests/SubtestDeploymentTrait.php.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-05-29T12:15:32.188Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in `.vortex/installer/tests/Fixtures/install` directory as they are test fixtures.
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-06-02T00:41:36.321Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1697
File: .vortex/installer/tests/Fixtures/install/provision_database_lagoon/docs/onboarding.md:16-17
Timestamp: 2025-06-02T00:41:36.321Z
Learning: DO NOT EVER REVIEW FILES IN .vortex/installer/tests/Fixtures/ directory - these are test fixtures and should not be reviewed under any circumstances.
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Use descriptive step definitions and discover available steps via 'ahoy test-bdd -- --definitions=l'
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Create supporting entities (taxonomies, media) in Behat Background section before content type tests to ensure test prerequisites exist
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Always prefix test content with [TEST] to distinguish test data from real content
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Test all visible fields in content type view scenarios to ensure proper display configuration
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Use numbered patterns for test content when creating multiple related test items: '[TEST] Topic 1', '[TEST] Topic 2'
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-06-01T08:09:18.993Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1693
File: .vortex/installer/tests/Fixtures/install/hosting_acquia/docs/deployment.md:6-8
Timestamp: 2025-06-01T08:09:18.993Z
Learning: Do not review files in test fixture directories (e.g., files under paths containing `/tests/Fixtures/` or similar test fixture patterns). These files are used for testing purposes and should remain as-is.
Applied to files:
.gitignore.artifact
🧬 Code graph analysis (1)
.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php (1)
.vortex/installer/src/Utils/File.php (1)
File(10-121)
⏰ 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). (11)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-common
🔇 Additional comments (18)
.vortex/tests/phpunit/Traits/SutTrait.php (7)
249-257: Redundant file assertions after directory assertion.Lines 251-256 assert specific files don't exist within
ys_base, but line 249 already asserts the entireys_basedirectory doesn't exist. While redundant, these explicit file checks may serve as documentation of the expected test structure. Consider whether the added clarity justifies the maintenance overhead.
259-268: LGTM!The updated absence assertions correctly verify the pre-installation state for example settings files, CI workflows, and documentation files.
285-292: LGTM!New assertions for database and Solr Docker configurations are consistent with the existing Docker file checks.
369-391: LGTM!Root configuration and documentation file assertions are correctly updated to reflect the expected project structure.
429-438: LGTM!The updated assertions correctly reflect the renamed test base classes from
YourSite*toSwBase*, following the project's naming conventions for the Star Wars demo.
454-461: LGTM!Settings file presence assertions correctly verify the expected Drupal configuration files.
463-483: LGTM!Theme file assertions correctly reflect the migration from Grunt to NPM scripts:
postcss.config.jsadded for PostCSS configurationpackage.jsonretained for NPM scripts- No Gruntfile.js assertion (correctly removed)
Test base file assertions follow the consistent naming pattern (
StarWars*TestBase)..vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php (3)
7-8: LGTM! Import necessary for test fixture creation.The import of
AlexSkrypnyk\File\Fileis required for theFile::dump()calls that create test fixtures for images and fonts.
24-26: LGTM! Test fixtures appropriately created to verify asset copying.The fixture files are created in source directories before the build process runs, allowing the subsequent assertions to verify that the NPM build scripts correctly copy assets from source to build output directories.
63-67: LGTM! Assertions verify the build process copies static assets.The existence checks confirm that the NPM build scripts successfully copy images and fonts from source directories to build output. The past review comment already suggested an optional enhancement to verify file content integrity, which remains a valid but non-blocking improvement.
.vortex/tests/phpunit/Traits/Subtests/SubtestDeploymentTrait.php (2)
94-94: LGTM! Correct update for the NPM-based build system.The assertion correctly verifies that
postcss.config.js(a development file) is excluded from deployment, replacing the previous check forGruntfile.js.
122-122: Update test assertions to use consistent minified JS signature across all test contexts.The test file expects
!function(e,Drupal){Drupal.behaviors.star_warsat line 122, but the same minified file is expected to have a different signature in SubtestDockerComposeTrait and SubtestAhoyTrait:!function(Drupal){"use strict";Drupal.behaviors.star_wars.These conflicting expectations cannot both be correct for the same generated artifact. Verify which minified signature is actually produced by the build process (using terser with
reserved=['jQuery','Drupal']) and ensure all test assertions match that output..vortex/installer/src/Prompts/Handlers/Theme.php (1)
238-244: LGTM! Correctly updates theme detection for NPM-based tooling.The removal of the
Gruntfile.jscheck aligns with the migration to NPM scripts. Theme detection now requires:
scss/_variables.scss(SCSS structure)package.json(NPM tooling)build-devscript inpackage.json(build capability)This relaxed criteria correctly reflects the new build system.
.dockerignore (1)
32-54: LGTM! Necessary additions for NPM-based tooling.The whitelisted files are all required for the new NPM-based build process:
.eslintrc.json,.prettierignore,.prettierrc.json: Linting and formatting configsgherkinlint.json: Gherkin linting configpackage.json,yarn.lock: Dependency managementpostcss.config.js: CSS processing configurationAll additions correctly support the migration from Grunt to NPM scripts.
.gitignore.artifact (1)
26-34: Artifact ignore patterns align with npm-based workflow.The updated ignore patterns correctly exclude npm-based tooling configuration and source files from deployment artifacts. Ignoring
package.json,yarn.lock,postcss.config.js, and.eslintrc.jsonis appropriate for an artifact deployment where only built assets are needed.web/themes/custom/your_site_theme/package.json (1)
39-39: Improved concat implementation addresses previous concerns.The
concatscript now usesfindwith-exec cat {} +instead of shell globbing, which is more robust and correctly filters out minified files. The removal of error suppression allows real failures to surface, addressing the previous review's concern about problematic fallback logic.web/themes/custom/your_site_theme/.eslintrc.json (1)
2-71: Comprehensive ESLint configuration aligns with modern tooling.The migration from a legacy extends reference to a full configuration is well-executed. The config appropriately combines Airbnb base rules, Prettier integration, YAML linting, JSDoc validation, and Drupal-specific globals. The rule customizations are sensible for Drupal theme development.
web/themes/custom/your_site_theme/README.md (1)
1-24: Clear documentation supports the npm-based workflow.The README effectively documents the theme's requirements, installation process, and available commands. The structure is clear and provides developers with the necessary information to work with the new npm-script-based tooling.
| * @file Global theme behaviors. | ||
| * @param $ | ||
| * @param Drupal | ||
| * @global Drupal, jQuery | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove redundant @param tags from file-level JSDoc.
File-level JSDoc blocks (with @file) typically don't include @param tags, as those belong on the function itself. The @param documentation is already correctly placed on the function at lines 11-12.
🔎 Proposed fix
/**
* @file Global theme behaviors.
- * @param $
- * @param Drupal
* @global Drupal, jQuery
*/📝 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.
| * @file Global theme behaviors. | |
| * @param $ | |
| * @param Drupal | |
| * @global Drupal, jQuery | |
| */ | |
| * @file Global theme behaviors. | |
| * @global Drupal, jQuery | |
| */ |
🤖 Prompt for AI Agents
In @web/themes/custom/your_site_theme/js/your_site_theme.js around lines 2-6,
The file-level JSDoc block contains redundant @param tags for $ and Drupal;
remove those @param entries from the top-level block (leaving @file and @global
as appropriate) and keep the existing @param tags on the actual function JSDoc
(the function that takes $ and Drupal) so parameter docs remain only on the
function-level comment.
| module.exports = { | ||
| plugins: [ | ||
| require('autoprefixer') | ||
| ] | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Standard PostCSS configuration.
The configuration correctly sets up autoprefixer, which is essential for adding vendor prefixes to CSS. This replaces the CSS processing previously handled by Grunt.
Optional enhancement: Consider adding cssnano for CSS minification if not handled elsewhere in the build pipeline:
module.exports = {
plugins: [
require('autoprefixer'),
require('cssnano')({
preset: 'default',
})
]
};🤖 Prompt for AI Agents
In @web/themes/custom/your_site_theme/postcss.config.js around lines 1-5, Add
CSS minification by updating the PostCSS plugins array in postcss.config.js to
include cssnano alongside the existing autoprefixer; require('cssnano') should
be added to the plugins list and invoked with the preset 'default' (i.e., pass
the config object to cssnano) so CSS output is minified during the PostCSS step.
This comment has been minimized.
This comment has been minimized.
|
|
|
AlexSkrypnyk
force-pushed
the
feature/replace-grunt-npm-scripts
branch
from
2044c43 to
ff2aafa
Compare
|
This comment has been minimized.
This comment has been minimized.
|
|
AlexSkrypnyk
force-pushed
the
feature/replace-grunt-npm-scripts
branch
from
ff2aafa to
92bf8fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @web/themes/custom/your_site_theme/package.json:
- Around line 6-29: Update the outdated devDependencies in package.json: bump
"eslint" from 8.57.1 to 9.39.2, "patch-package" from 6.5.1 to 8.0.1 (or later to
fix the tmp vulnerability), "stylelint-config-standard" from 36.0.1 to 39.0.1,
and "stylelint-config-standard-scss" from 13.1.0 to 16.0.0; after changing these
entries run your package manager (yarn install), then run yarn audit or npm
audit and your test/build pipeline to verify nothing breaks and that the tmp
vulnerability is resolved by the updated patch-package.
♻️ Duplicate comments (11)
web/themes/custom/your_site_theme/.eslintrc.json (1)
35-35: Fix inconsistent spacing in object property (duplicate).The extra space before the colon in
"bodyScrollLock" : truewas previously flagged but remains unaddressed. Please update to"bodyScrollLock": trueto match the formatting of other properties.web/themes/custom/your_site_theme/js/your_site_theme.js (3)
2-6: Remove redundant @param tags from file-level JSDoc (duplicate).The file-level JSDoc block still contains @param tags for $ and Drupal that were previously flagged. These should be removed as they belong only on the function JSDoc (lines 11-12).
17-17: Remove placeholder comment (duplicate - inconsistent with claimed fix).The placeholder comment "give me example code here that would be using context with body" was previously flagged and marked as addressed in commit c49f226, but it's still present in the code. Please remove this development artifact.
16-25: Fix incorrect body element selection logic (duplicate - critical issue).The attach method still contains the previously flagged critical errors:
$(context).find('body')won't work correctly—body is rarely a descendant of the context parameter- jQuery
.once()is deprecated in Drupal 10+; use the standaloneonce()function- Should use
once()from the once library directlyThis affects the fundamental behavior of the theme JavaScript.
🔎 Recommended fix using modern Drupal once() API
attach(context) { - // give me example code here that would be using context with body - $(context) - .find('body') - .once('your-site-theme') - .each(function iterateBody() { - // Example: Add a class to the body element. - $(this).addClass('your-site-theme-processed'); - }); + const bodyElements = once('your-site-theme', 'body', context); + bodyElements.forEach((element) => { + // Example: Add a class to the body element. + element.classList.add('your-site-theme-processed'); + }); },web/themes/custom/your_site_theme/postcss.config.js (1)
1-5: Previous review comment addressed CSS minification.The PostCSS configuration is correct. A previous review already suggested considering cssnano for CSS minification if not handled elsewhere in the build pipeline.
web/themes/custom/your_site_theme/README.md (1)
1-24: Documentation looks good.The README clearly documents the npm-based workflow with installation instructions and available commands. A previous review suggested improving consistency in the command descriptions table (lines 23-24).
.vortex/tests/phpunit/Traits/SutTrait.php (1)
336-356: Duplicate assertion detected.Lines 347-348 both assert the existence of
scripts/vortex/login-container-registry.sh. This appears to be a duplicate that should be removed.🔎 Remove duplicate assertion
$this->assertFileExists('scripts/vortex/login-container-registry.sh'); - $this->assertFileExists('scripts/vortex/login-container-registry.sh'); $this->assertFileExists('scripts/vortex/login.sh');web/themes/custom/your_site_theme/package.json (4)
54-54: Error suppression masks build failures.The
|| truepattern suppresses all terser errors, including cases where the input file doesn't exist (ifconcatfailed). This makes debugging build issues difficult and can lead to incomplete builds passing silently.🔎 Remove error suppression for visibility
- "uglify": "terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal'] 2>/dev/null || true", + "uglify": "terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal']",If you need to handle missing JS files gracefully (e.g., themes with no custom JS), add an explicit check:
- "uglify": "terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal'] 2>/dev/null || true", + "uglify": "test -f build/js/your_site_theme.min.js && terser build/js/your_site_theme.min.js -o build/js/your_site_theme.min.js -c drop_console=true -m reserved=['jQuery','Drupal'] || echo 'No JS to minify'",
55-55: Consider adding debouncing to watch command.The watch script rebuilds on every file change, potentially triggering multiple builds if several files are saved quickly (e.g., auto-save, mass formatting).
🔎 Add debouncing for better performance
- "watch": "chokidar 'js/**/*.js' 'scss/**/*.scss' -c 'npm run build-dev'", + "watch": "chokidar 'js/**/*.js' 'scss/**/*.scss' --debounce 500 --initial -c 'npm run build-dev'",The
--debounce 500flag waits 500ms after the last change before running the build. The--initialflag runs the build once when watch starts.
52-52: Consider more accurate filename for dev build.The
sass:devscript outputs expanded CSS toyour_site_theme.min.css, which is misleading since the file isn't actually minified. This could confuse developers debugging styles.🔎 Alternative naming approaches
Option 1: Use a non-minified filename for dev builds:
- "sass:dev": "sass scss/styles.scss build/css/your_site_theme.min.css --source-map --style=expanded", + "sass:dev": "sass scss/styles.scss build/css/your_site_theme.css --source-map --style=expanded",Then update
postcss:devand any theme configuration to reference the non-minified name.Option 2: Keep the current approach if Drupal theme configuration expects a single filename across environments.
41-42: Error suppression may mask legitimate failures.The
2>/dev/null || truepattern suppresses all errors, including permission issues or disk space problems. While gracefully handling missing source directories is reasonable, other failures should be visible.🔎 Improved approach with better error visibility
- "copy:fonts": "mkdir -p build/fonts && cp -r fonts/* build/fonts/ 2>/dev/null || true", - "copy:images": "mkdir -p build/images && cp -r images/* build/images/ 2>/dev/null || true", + "copy:fonts": "mkdir -p build/fonts && (test -d fonts && cp -r fonts/* build/fonts/ || echo 'No fonts directory to copy')", + "copy:images": "mkdir -p build/images && (test -d images && cp -r images/* build/images/ || echo 'No images directory to copy')",This approach explicitly checks for directory existence and provides feedback when directories are missing, while allowing real errors (permissions, disk space) to surface.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (83)
.vortex/installer/tests/Fixtures/handler_process/_baseline/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/_baseline/web/themes/custom/star_wars/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_all_circleci/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_all_gha/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/deploy_types_artifact/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/themes/custom/star_wars/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/.gitignore.artifactis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/js/star_wars.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/themes/custom/star_wars/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/js/lightsaber.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/lightsaber/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/names/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/provision_database_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_claro/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_claro/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/js/light_saber.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/light_saber/scss/_rem.scssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_custom/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_olivero/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_olivero/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_stark/web/themes/custom/star_wars/-README.mdis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/theme_stark/web/themes/custom/star_wars/-postcss.config.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_lint_circleci/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_tests/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_tests_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_behat/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_behat_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpcs_circleci/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpmd/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpmd_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpstan/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpstan_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpunit/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_phpunit_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_rector/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_no_rector_circleci/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_none/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/tools_none/web/themes/custom/star_wars/Gruntfile.jsis excluded by!.vortex/installer/tests/Fixtures/**web/themes/custom/your_site_theme/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
.dockerignore.gitignore.artifact.vortex/docs/content/drupal/theme-scaffold.mdx.vortex/installer/src/Prompts/Handlers/Theme.php.vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php.vortex/tests/phpunit/Traits/Subtests/SubtestAhoyTrait.php.vortex/tests/phpunit/Traits/Subtests/SubtestDeploymentTrait.php.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php.vortex/tests/phpunit/Traits/SutTrait.phpweb/themes/custom/your_site_theme/.eslintrc.jsonweb/themes/custom/your_site_theme/Gruntfile.jsweb/themes/custom/your_site_theme/README.mdweb/themes/custom/your_site_theme/js/your_site_theme.jsweb/themes/custom/your_site_theme/package.jsonweb/themes/custom/your_site_theme/postcss.config.jsweb/themes/custom/your_site_theme/scss/_rem.scss
💤 Files with no reviewable changes (3)
- .vortex/docs/content/drupal/theme-scaffold.mdx
- .vortex/installer/tests/Unit/Handlers/AbstractHandlerDiscoveryTestCase.php
- web/themes/custom/your_site_theme/Gruntfile.js
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-09-13T04:14:41.765Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2011
File: .vortex/tests/phpunit/Traits/Steps/StepAhoyTrait.php:20-23
Timestamp: 2025-09-13T04:14:41.765Z
Learning: In .vortex/tests/phpunit PHPUnit tests using cmd/cmdFail helpers, patterns in the expected output arrays support '*' markers for positive assertions (text must be present) and '!' markers for negative assertions (text must NOT be present). The '*' markers are NOT literal text matches but rather indicate expected presence.
Applied to files:
.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php.vortex/tests/phpunit/Traits/Subtests/SubtestAhoyTrait.php.vortex/tests/phpunit/Traits/SutTrait.php.vortex/tests/phpunit/Traits/Subtests/SubtestDeploymentTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Always prefix test content with [TEST] to distinguish test data from real content
Applied to files:
.vortex/tests/phpunit/Traits/Subtests/SubtestAhoyTrait.php.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-05-29T12:15:32.188Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in `.vortex/installer/tests/Fixtures/install` directory as they are test fixtures.
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-06-02T00:41:36.321Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1697
File: .vortex/installer/tests/Fixtures/install/provision_database_lagoon/docs/onboarding.md:16-17
Timestamp: 2025-06-02T00:41:36.321Z
Learning: DO NOT EVER REVIEW FILES IN .vortex/installer/tests/Fixtures/ directory - these are test fixtures and should not be reviewed under any circumstances.
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Use descriptive step definitions and discover available steps via 'ahoy test-bdd -- --definitions=l'
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Create supporting entities (taxonomies, media) in Behat Background section before content type tests to ensure test prerequisites exist
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Test all visible fields in content type view scenarios to ensure proper display configuration
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-11-24T21:29:10.896Z
Learnt from: CR
Repo: drevops/vortex PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T21:29:10.896Z
Learning: Applies to tests/behat/features/**/*.feature : Use numbered patterns for test content when creating multiple related test items: '[TEST] Topic 1', '[TEST] Topic 2'
Applied to files:
.vortex/tests/phpunit/Traits/SutTrait.php
📚 Learning: 2025-07-23T01:16:30.963Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as `required=true` in Docker build mounts. The conditional check `if [ -s /run/secrets/github_token ]` is the preferred approach to allow builds to proceed without a token when needed.
Applied to files:
web/themes/custom/your_site_theme/package.json
📚 Learning: 2025-06-01T08:09:18.993Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1693
File: .vortex/installer/tests/Fixtures/install/hosting_acquia/docs/deployment.md:6-8
Timestamp: 2025-06-01T08:09:18.993Z
Learning: Do not review files in test fixture directories (e.g., files under paths containing `/tests/Fixtures/` or similar test fixture patterns). These files are used for testing purposes and should remain as-is.
Applied to files:
.gitignore.artifact
⏰ 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). (11)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.4)
🔇 Additional comments (14)
.dockerignore (1)
32-54: LGTM! Whitelist correctly updated for NPM-based tooling.The Docker ignore whitelist has been properly updated to include all necessary configuration files for the npm-based workflow (.eslintrc.json, prettier configs, postcss.config.js) while appropriately excluding Gruntfile.js.
.gitignore.artifact (1)
26-34: LGTM! Deployment artifact ignore entries correctly updated.The ignore entries have been properly updated to exclude npm-based development files (.eslintrc.json, node_modules, postcss.config.js, scss) from the deployment artifact, aligning with the migration from Grunt to npm scripts.
.vortex/tests/phpunit/Traits/Subtests/SubtestDeploymentTrait.php (2)
94-94: Correct update for npm-based tooling.The assertion has been properly updated to check for
postcss.config.jsinstead ofGruntfile.js, correctly reflecting the migration from Grunt to npm-based build tooling.
122-122: Same assertion loosening as SubtestAhoyTrait.php.This assertion has been relaxed to a generic
!function(pattern, matching the change in SubtestAhoyTrait.php line 56. While this validates minification, it's less specific about the Drupal behavior structure.Verify that this generic pattern is sufficient for your testing needs and that the theme's JS structure has changed to warrant this adjustment.
.vortex/tests/phpunit/Traits/Subtests/SubtestAhoyTrait.php (1)
56-56: No action needed. The assertion checking for'!function('in the minified JS was newly introduced in the commit "Replaced Grunt with NPM scripts in demo theme" and is not a loosening of a previous pattern. This is a legitimate smoke test verifying that minification occurred, using a common pattern from standard JavaScript minifiers like UglifyJS or Terser.Likely an incorrect or invalid review comment.
.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php (2)
60-60: LGTM! Generic pattern improves test resilience.The updated assertion now checks for a generic minified wrapper pattern (
!function() rather than specific content, making the test more resilient to variations in minifier output.
63-67: LGTM! Asset copy verification is functional.The assertions correctly verify that build scripts copy static assets (images and fonts) to the build directory. Existence checks are appropriate for this level of integration testing.
web/themes/custom/your_site_theme/package.json (1)
39-39: LGTM! Improved concat implementation.The
find-based approach is more robust than shell globbing patterns. It will fail appropriately if thejsdirectory is missing, providing clear error feedback rather than silently creating an empty file..vortex/tests/phpunit/Traits/SutTrait.php (6)
251-268: LGTM! Updated absence assertions reflect template cleanup.The additional absence checks correctly verify that legacy template files (YourSite test bases, example settings, documentation) are not present before Vortex installation.
287-292: LGTM! Added Docker configuration files.The additions of
.docker/config/database/my.cnfand.docker/solr.dockerfilecorrectly reflect the expected Vortex file structure.
370-388: LGTM! Updated root configuration file expectations.The additions correctly reflect new configuration files (
.ahoy.local.example.yml,docs/faqs.md) and the removal of.gitattributesfrom the expected file set.
430-438: LGTM! Module test structure correctly verified.The assertions properly verify the presence of the generated module structure, including deploy hooks and test base classes for all test types (Functional, Kernel, Unit).
457-460: LGTM! Settings file assertions updated.The assertions correctly verify the presence of Drupal settings files after installation, including scaffolded defaults and example local settings.
469-482: LGTM! Theme structure reflects npm-based workflow.The assertions correctly verify the migrated theme structure, including npm configuration (
package.json,postcss.config.js) and test base classes for all test types.
| protected static function isVortexTheme(string $dir): bool { | ||
| $c1 = file_exists($dir . '/scss/_variables.scss'); | ||
| $c2 = file_exists($dir . '/Gruntfile.js'); | ||
| $c3 = file_exists($dir . '/package.json'); | ||
| $c4 = File::contains($dir . '/package.json', 'build-dev'); | ||
| $c2 = file_exists($dir . '/package.json'); | ||
| $c3 = File::contains($dir . '/package.json', 'build-dev'); | ||
|
|
||
| return $c1 && $c2 && $c3 && $c4; | ||
| return $c1 && $c2 && $c3; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider more robust JSON parsing for theme detection.
The current string search for 'build-dev' in package.json works but could theoretically match in comments or string values. For more robust detection, consider parsing the JSON and checking for the existence of scripts.build-dev.
🔎 Optional improvement using JSON parsing
protected static function isVortexTheme(string $dir): bool {
$c1 = file_exists($dir . '/scss/_variables.scss');
$c2 = file_exists($dir . '/package.json');
- $c3 = File::contains($dir . '/package.json', 'build-dev');
+ $c3 = false;
+ if ($c2) {
+ $package = json_decode(file_get_contents($dir . '/package.json'), true);
+ $c3 = isset($package['scripts']['build-dev']);
+ }
return $c1 && $c2 && $c3;
}| "devDependencies": { | ||
| "@lodder/grunt-postcss": "^3.1.1", | ||
| "autoprefixer": "^10.4.22", | ||
| "chokidar-cli": "^3.0.0", | ||
| "eslint": "^8.57.1", | ||
| "grunt": "^1.6.1", | ||
| "grunt-contrib-clean": "^2.0.1", | ||
| "grunt-contrib-concat": "^2.1.0", | ||
| "grunt-contrib-copy": "^1.0.0", | ||
| "grunt-contrib-uglify": "^5.2.2", | ||
| "grunt-contrib-watch": "^1.1.0", | ||
| "grunt-exec": "^3.0.0", | ||
| "grunt-sass": "^4.0.1", | ||
| "grunt-stylelint": "^0.20.1", | ||
| "gruntify-eslint": "^5.0.0", | ||
| "eslint-config-airbnb-base": "^15.0.0", | ||
| "eslint-config-prettier": "^10.1.8", | ||
| "eslint-plugin-import": "^2.32.0", | ||
| "eslint-plugin-jsdoc": "^61.5.0", | ||
| "eslint-plugin-no-jquery": "^3.1.1", | ||
| "eslint-plugin-prettier": "^5.5.4", | ||
| "eslint-plugin-yml": "^1.19.0", | ||
| "patch-package": "^6.5.1", | ||
| "postcss": "^8.5.6", | ||
| "postcss-cli": "^11.0.1", | ||
| "prettier": "^3.7.4", | ||
| "prettier-plugin-jsdoc": "^1.8.0", | ||
| "sass": "^1.94.2", | ||
| "stylelint": "^16.26.1", | ||
| "stylelint-config-standard": "^36.0.1", | ||
| "stylelint-config-standard-scss": "^13.1.0", | ||
| "stylelint-scss": "^6.12.1" | ||
| "stylelint-order": "^7.0.1", | ||
| "stylelint-scss": "^6.12.1", | ||
| "terser": "^5.39.0" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd web/themes/custom/your_site_theme && npm outdated && npm auditRepository: drevops/vortex
Length of output: 614
🏁 Script executed:
# Check if package-lock.json exists
find . -name "package-lock.json" -o -name "yarn.lock" -o -name "npm-shrinkwrap.json" | head -20
# Verify the package.json file exists at the specified path
ls -la web/themes/custom/your_site_theme/package.json
# Check the directory structure
ls -la web/themes/custom/your_site_theme/Repository: drevops/vortex
Length of output: 1646
🏁 Script executed:
cd web/themes/custom/your_site_theme && yarn outdated && echo "---" && yarn audit --level moderateRepository: drevops/vortex
Length of output: 1307
🏁 Script executed:
cd web/themes/custom/your_site_theme && yarn auditRepository: drevops/vortex
Length of output: 1604
Update outdated devDependencies and address low-severity vulnerability.
The project uses yarn for dependency management. Verification found 4 packages with available major updates:
eslint: 8.57.1 → 9.39.2patch-package: 6.5.1 → 8.0.1stylelint-config-standard: 36.0.1 → 39.0.1stylelint-config-standard-scss: 13.1.0 → 16.0.0
A low-severity vulnerability was also detected in the tmp package (transitive dependency of patch-package), which allows arbitrary temporary file/directory writes via symbolic link. Updating patch-package to 8.0.1 or later will resolve this.
🤖 Prompt for AI Agents
In @web/themes/custom/your_site_theme/package.json around lines 6-29, Update the
outdated devDependencies in package.json: bump "eslint" from 8.57.1 to 9.39.2,
"patch-package" from 6.5.1 to 8.0.1 (or later to fix the tmp vulnerability),
"stylelint-config-standard" from 36.0.1 to 39.0.1, and
"stylelint-config-standard-scss" from 13.1.0 to 16.0.0; after changing these
entries run your package manager (yarn install), then run yarn audit or npm
audit and your test/build pipeline to verify nothing breaks and that the tmp
vulnerability is resolved by the updated patch-package.
|
|
|
|
Summary by CodeRabbit
New Features
Chores
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.