[#1754] Added JS and CSS example libraries to the ys_base and added module CSS and JS linting. #1787
Conversation
WalkthroughThis update introduces a new counter block feature in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Drupal
participant CounterBlock (PHP)
participant Twig Template
participant JS Behavior
participant LocalStorage
User->>Drupal: Loads homepage
Drupal->>CounterBlock: Renders counter block
CounterBlock->>Twig Template: Passes counter_value=0
Twig Template->>User: Displays counter block HTML
User->>JS Behavior: Interacts with counter buttons
JS Behavior->>LocalStorage: Reads/writes counter value
JS Behavior->>User: Updates counter display with animation
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected related to the objectives from the linked issues. Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 phpcs (3.7.2)web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information web/modules/custom/ys_base/ys_base.deploy.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information 🔧 ESLint
.vortex/docs/.eslintrc.jsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "/.vortex/docs".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in ".vortex/docs/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1787 +/- ##
===========================================
+ Coverage 72.44% 72.47% +0.03%
===========================================
Files 82 83 +1
Lines 4634 4676 +42
Branches 35 35
===========================================
+ Hits 3357 3389 +32
- Misses 1277 1287 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (1)
web/modules/custom/ys_base/ys_base.libraries.yml (1)
1-20: Consider refactoring to eliminate redundant library definitions.Both
generalandcounterlibraries define identical assets (css/ys_base.cssandjs/ys_base.js). This redundancy could lead to confusion and potential duplicate loading if both libraries are attached to the same page.Consider one of these approaches:
- Use a single library if the assets are truly shared
- Split the assets if they serve different purposes
- Use library dependencies if one builds upon the other
-general: - version: 1.x - css: - theme: - css/ys_base.css: {} - js: - js/ys_base.js: {} - dependencies: - - core/drupal - -counter: - version: 1.x - css: - theme: - css/ys_base.css: {} - js: - js/ys_base.js: {} - dependencies: - - core/drupal +base: + version: 1.x + css: + theme: + css/ys_base.css: {} + js: + js/ys_base.js: {} + dependencies: + - core/drupal + +counter: + version: 1.x + dependencies: + - ys_base/baseAlso consider using more specific versioning (e.g.,
1.0.0) for better cache control.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (58)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.eslintignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.stylelintrc.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/phpcs.xmlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/tests/behat/features/counter.featureis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/css/sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/js/sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/src/Plugin/Block/CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/sw_base.moduleis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/templates/ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/ciprovider_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_all_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deps_updates_provider_ci_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/.eslintignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/css/sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/js/sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/src/Plugin/Block/CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/sw_base.moduleis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/templates/ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/-sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/css/-sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/js/-sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/src/Plugin/Block/-CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/templates/-ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/-sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/css/-sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/js/-sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/src/Plugin/Block/-CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/templates/-ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/css/the_force_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/js/the_force_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/src/Plugin/Block/CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/templates/ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/the_force_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/the_force_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/the_force_base.moduleis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_database_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_profile/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/web/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_custom/web/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
.ahoy.yml(4 hunks).circleci/config.yml(2 hunks).dockerignore(1 hunks).eslintignore(1 hunks).eslintrc.json(1 hunks).github/workflows/build-test-deploy.yml(2 hunks).stylelintrc.js(1 hunks).vortex/tests/bats/_helper.workflow.bash(3 hunks)package.json(1 hunks)phpcs.xml(1 hunks)tests/behat/features/counter.feature(1 hunks)web/modules/custom/ys_base/css/ys_base.css(1 hunks)web/modules/custom/ys_base/js/ys_base.js(1 hunks)web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php(1 hunks)web/modules/custom/ys_base/templates/ys-base-counter-block.html.twig(1 hunks)web/modules/custom/ys_base/ys_base.deploy.php(1 hunks)web/modules/custom/ys_base/ys_base.libraries.yml(1 hunks)web/modules/custom/ys_base/ys_base.module(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/behat/**/*.feature
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (7)
.dockerignore (1)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
.eslintignore (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.github/workflows/build-test-deploy.yml (2)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Composer dependencies should be updated automatically with compatibility checks
tests/behat/features/counter.feature (3)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Applies to tests/behat/**/*.feature : Use consistent user types in Behat feature files: 'site visitor', 'site administrator', and 'content editor'
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Applies to tests/behat/**/*.feature : All Behat feature files must use the standard user story format: 'As a [user type]\nI want to [action]\nSo that [benefit]'
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Applies to tests/behat/**/*.feature : Do not use commas or periods in user story statements within Behat feature files
package.json (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
web/modules/custom/ys_base/ys_base.libraries.yml (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.circleci/config.yml (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Composer dependencies should be updated automatically with compatibility checks
🪛 Biome (1.9.4)
web/modules/custom/ys_base/js/ys_base.js
[error] 7-7: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
[error] 69-69: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 36-36: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ 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). (12)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
🔇 Additional comments (26)
.dockerignore (1)
32-34: LGTM! Configuration files properly whitelisted for linting.These additions ensure that ESLint and Stylelint configuration files are available in the Docker build context, which is essential for the new frontend linting infrastructure to work properly.
phpcs.xml (1)
16-16: LGTM! Appropriate separation of concerns for code linting.Removing JavaScript and CSS extensions from PHPCS is the correct approach since dedicated linting tools (ESLint and Stylelint) are now handling these file types. This prevents duplicate linting and allows each tool to focus on its specialty.
.eslintignore (1)
1-10: LGTM! Comprehensive and appropriate ESLint ignore configuration.The ignore patterns correctly exclude third-party code, core Drupal files, generated content, and minified assets from linting. This is a standard and well-structured configuration for a Drupal project.
web/modules/custom/ys_base/ys_base.module (1)
24-33: LGTM! Theme hook implementation follows Drupal standards.The theme hook registration is properly structured with appropriate variable defaults and template naming convention. This will work correctly with the Twig template system.
web/modules/custom/ys_base/ys_base.deploy.php (1)
17-46: LGTM - Well-structured deployment function.The function correctly handles the block creation workflow with proper error checking and uses Drupal's entity API appropriately.
.stylelintrc.js (1)
1-39: LGTM - Well-configured Stylelint setup.The configuration appropriately extends the standard config, includes property ordering, and correctly ignores Sass/SCSS at-rules. The disabled rules provide flexibility while maintaining code quality.
package.json (2)
18-26: LGTM - Appropriate linting dependencies.The ESLint and Stylelint configurations with their respective plugins are well-chosen for modern JavaScript and CSS development.
6-9: Node.js and Yarn versions verified and supportedThe project's engines requirement (
node >= 20.0,yarn >= 1.22.22) aligns with current releases and LTS practices. Local checks show:
- Node v24.3.0 (latest v24.4.1, LTS remains v20.x)
- Yarn 1.22.22
No updates to the version constraints are needed.
web/modules/custom/ys_base/templates/ys-base-counter-block.html.twig (1)
1-26: LGTM - Excellent template structure and accessibility.The template demonstrates best practices with:
- Semantic HTML structure
- Proper data attributes for JavaScript hooks
- Accessibility attributes (aria-label)
- Clear separation between display and controls
- Comprehensive documentation
web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php (2)
25-35: LGTM - Proper block plugin implementation.The block plugin correctly implements the required methods, uses appropriate theme hooks, and attaches the necessary JavaScript/CSS library.
40-43: Review Caching Strategy for CounterBlockI only found this one block plugin overriding getCacheMaxAge (web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php). By returning 0 you disable all caching, which could impact performance even if the interactive behavior is client-side.
• File: web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php
– getCacheMaxAge() returns 0 (no cache)
• No other block plugins override caching in the codebasePlease confirm whether the counter really needs zero caching, or if you can:
- Enable a non-infinite max age for the initial render
- Leverage Drupal cache contexts (e.g. user, URL) or tags to scope cache entries
.github/workflows/build-test-deploy.yml (4)
254-254: LGTM: NodeJS dependencies installation added.The
yarn install --frozen-lockcommand ensures reproducible builds by using the exact versions specified in the lockfile.
256-256: LGTM: Grammatical improvement.The step name is now grammatically correct.
284-286: LGTM: Module linting step added.The separation of module and theme linting steps improves clarity and allows for different linting configurations.
289-289: LGTM: Clear step naming.The step name now clearly indicates it's for theme-specific linting.
.circleci/config.yml (4)
266-266: LGTM: Consistent NodeJS dependency management.The yarn install step maintains consistency with the GitHub Actions workflow.
269-269: LGTM: Grammatical improvement.Consistent with the GitHub Actions workflow improvement.
297-298: LGTM: Module linting step added.The module linting step is properly implemented with failure handling.
300-304: LGTM: Theme linting step added.The theme linting step is properly scoped to the theme directory and includes proper error handling.
tests/behat/features/counter.feature (2)
4-6: LGTM: Proper user story format.The user story follows the required format with correct user type and no punctuation as specified in the guidelines.
9-40: LGTM: Comprehensive test coverage.The scenarios effectively test all counter functionality including basic operations, negative values, reset functionality, and persistence across page reloads.
web/modules/custom/ys_base/js/ys_base.js (1)
9-74: LGTM: Well-structured Drupal behavior.The counter implementation follows Drupal JavaScript patterns correctly with proper context handling, event management, and localStorage integration.
web/modules/custom/ys_base/css/ys_base.css (3)
7-14: LGTM: Well-structured block container.The counter block has good visual hierarchy with proper spacing and borders.
28-41: LGTM: Excellent interactive feedback.The counter value styling with transition effects and updated state provides excellent user feedback.
50-100: LGTM: Comprehensive button styling.The button styles provide clear visual differentiation for different actions with good hover and active states.
.eslintrc.json (1)
199-208:valid-jsdocis deprecated – switch to eslint-plugin-jsdocThe core rule was dropped; current ESLint will error if it’s referenced.
Migrate to the maintainedeslint-plugin-jsdocor remove the entry to keep the configuration portable.- "valid-jsdoc": [ - "warn", - { - … - } - ] + "jsdoc/valid-jsdoc": "warn"(Remember to add
"plugins": ["jsdoc"]).
| function ys_base_page_attachments_alter(array &$attachments): void { | ||
| $attachments['#attached']['library'][] = 'ys_base/general'; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider performance implications of global library attachment.
Attaching the ys_base/general library to all pages may impact performance if the assets are only needed on specific pages. Consider whether this library is truly needed globally or if it should be attached conditionally.
If the library isn't needed on all pages, consider conditional attachment:
function ys_base_page_attachments_alter(array &$attachments): void {
- $attachments['#attached']['library'][] = 'ys_base/general';
+ // Only attach on specific routes or conditions
+ $route_name = \Drupal::routeMatch()->getRouteName();
+ if (in_array($route_name, ['page_manager.page_view_node', 'entity.node.canonical'])) {
+ $attachments['#attached']['library'][] = 'ys_base/general';
+ }
}📝 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.
| function ys_base_page_attachments_alter(array &$attachments): void { | |
| $attachments['#attached']['library'][] = 'ys_base/general'; | |
| } | |
| function ys_base_page_attachments_alter(array &$attachments): void { | |
| // Only attach on specific routes or conditions | |
| $route_name = \Drupal::routeMatch()->getRouteName(); | |
| if (in_array($route_name, ['page_manager.page_view_node', 'entity.node.canonical'])) { | |
| $attachments['#attached']['library'][] = 'ys_base/general'; | |
| } | |
| } |
🤖 Prompt for AI Agents
In web/modules/custom/ys_base/ys_base.module at lines 15 to 17, the
ys_base/general library is attached globally to all pages, which may cause
unnecessary performance overhead. Modify the ys_base_page_attachments_alter
function to attach the library conditionally based on specific page routes,
content types, or other criteria where the assets are actually needed, instead
of attaching it unconditionally to every page.
| /** | ||
| * Implements hook_theme(). | ||
| * | ||
| * @phpstan-ignore-next-line |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the purpose of the PHPStan ignore comment.
The @phpstan-ignore-next-line comment suggests there's a type issue with the ys_base_theme() function. Please verify if this is still necessary or if the underlying issue can be resolved.
🏁 Script executed:
#!/bin/bash
# Check if there are any PHPStan configuration files or recent changes that might explain this ignore
fd -e neon -e yaml -e yml | grep -i phpstanLength of output: 58
🏁 Script executed:
#!/bin/bash
# Display the lines around the @phpstan-ignore-next-line to inspect the function signature and context
rg -n "@phpstan-ignore-next-line" -A5 web/modules/custom/ys_base/ys_base.moduleLength of output: 231
🏁 Script executed:
#!/bin/bash
# Locate the PHPStan binary, if available, and run an analysis on the ys_base.module file
if [[ -x "vendor/bin/phpstan" ]]; then
echo "Running PHPStan on ys_base.module…"
vendor/bin/phpstan analyse web/modules/custom/ys_base/ys_base.module --level max --error-format=raw || true
else
echo "PHPStan binary not found at vendor/bin/phpstan"
fiLength of output: 132
Update hook_theme signature and remove ignore
The @phpstan-ignore-next-line is silencing a signature mismatch: PHPStan expects the standard hook_theme parameters. Instead of ignoring it, update the function to match Drupal’s hook definition and drop the ignore.
• File: web/modules/custom/ys_base/ys_base.module
– Remove the @phpstan-ignore-next-line on line 22.
– Change the signature on line 24 to include the four hook parameters.
Suggested diff:
- * @phpstan-ignore-next-line
- */
- function ys_base_theme(): array {
+ */
+ /**
+ * Implements hook_theme().
+ *
+ * @param array $existing
+ * Existing theme implementations.
+ * @param string $type
+ * The type of hook.
+ * @param string $theme
+ * Theme.
+ * @param string $path
+ * Module path.
+ *
+ * @return array
+ * Theme hook definitions.
+ */
+ function ys_base_theme(array $existing, string $type, string $theme, string $path): array {🤖 Prompt for AI Agents
In web/modules/custom/ys_base/ys_base.module at lines 22 to 24, remove the
@phpstan-ignore-next-line comment on line 22 and update the hook_theme function
signature on line 24 to include all four standard parameters as defined by
Drupal's hook_theme. This will align the function with the expected signature
and eliminate the need for the ignore comment.
| /** | ||
| * {@inheritdoc} | ||
| * | ||
| * @phpstan-ignore-next-line |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unnecessary phpstan-ignore comment.
The @phpstan-ignore-next-line comment appears unnecessary for the build() method override. This comment should only be used when there's a legitimate phpstan issue that cannot be resolved otherwise.
- /**
- * {@inheritdoc}
- *
- * @phpstan-ignore-next-line
- */
+ /**
+ * {@inheritdoc}
+ */📝 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.
| * @phpstan-ignore-next-line | |
| /** | |
| * {@inheritdoc} | |
| */ |
🤖 Prompt for AI Agents
In web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php at line 23,
remove the unnecessary @phpstan-ignore-next-line comment above the build()
method override, as there is no unresolved phpstan issue requiring it.
| background: #f9f9f9; | ||
| border: 1px solid #ddd; | ||
| border-radius: 8px; | ||
| max-width: 300px; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider responsive design.
The fixed max-width: 300px might not work well on very small screens. Consider using relative units or media queries.
- max-width: 300px;
+ max-width: min(300px, 90vw);📝 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.
| max-width: 300px; | |
| max-width: min(300px, 90vw); |
🤖 Prompt for AI Agents
In web/modules/custom/ys_base/css/ys_base.css at line 11, the fixed max-width of
300px may not be suitable for very small screens. Replace the fixed pixel value
with relative units like percentages or viewport widths, or add media queries to
adjust the max-width based on screen size for better responsive design.
| "new-parens": "error", | ||
| "no-array-constructor": "error", | ||
| "no-caller": "error", | ||
| "no-catch-shadow": "error", | ||
| "no-eval": "error", | ||
| "no-extend-native": "error", | ||
| "no-extra-bind": "error", | ||
| "no-extra-parens": [ | ||
| "error", | ||
| "functions" | ||
| ], | ||
| "no-implied-eval": "error", | ||
| "no-iterator": "error", | ||
| "no-label-var": "error", | ||
| "no-labels": "error", | ||
| "no-lone-blocks": "error", | ||
| "no-loop-func": "error", | ||
| "no-multi-spaces": "error", | ||
| "no-multi-str": "error", | ||
| "no-native-reassign": "error", | ||
| "no-nested-ternary": "error", | ||
| "no-new-func": "error", | ||
| "no-new-object": "error", | ||
| "no-new-wrappers": "error", | ||
| "no-octal-escape": "error", |
There was a problem hiding this comment.
Deprecated / removed ESLint rule no-native-reassign
no-native-reassign was replaced by no-global-assign and has been removed from ESLint ≥ v4.
Keeping it causes “Configuration for rule … not found” and the linter exits with code 2.
- "no-native-reassign": "error",
+ "no-global-assign": "error",Ensure CI uses an ESLint version that supports every declared rule.
📝 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.
| "new-parens": "error", | |
| "no-array-constructor": "error", | |
| "no-caller": "error", | |
| "no-catch-shadow": "error", | |
| "no-eval": "error", | |
| "no-extend-native": "error", | |
| "no-extra-bind": "error", | |
| "no-extra-parens": [ | |
| "error", | |
| "functions" | |
| ], | |
| "no-implied-eval": "error", | |
| "no-iterator": "error", | |
| "no-label-var": "error", | |
| "no-labels": "error", | |
| "no-lone-blocks": "error", | |
| "no-loop-func": "error", | |
| "no-multi-spaces": "error", | |
| "no-multi-str": "error", | |
| "no-native-reassign": "error", | |
| "no-nested-ternary": "error", | |
| "no-new-func": "error", | |
| "no-new-object": "error", | |
| "no-new-wrappers": "error", | |
| "no-octal-escape": "error", | |
| "new-parens": "error", | |
| "no-array-constructor": "error", | |
| "no-caller": "error", | |
| "no-catch-shadow": "error", | |
| "no-eval": "error", | |
| "no-extend-native": "error", | |
| "no-extra-bind": "error", | |
| "no-extra-parens": [ | |
| "error", | |
| "functions" | |
| ], | |
| "no-implied-eval": "error", | |
| "no-iterator": "error", | |
| "no-label-var": "error", | |
| "no-labels": "error", | |
| "no-lone-blocks": "error", | |
| "no-loop-func": "error", | |
| "no-multi-spaces": "error", | |
| "no-multi-str": "error", | |
| - "no-native-reassign": "error", | |
| + "no-global-assign": "error", | |
| "no-nested-ternary": "error", | |
| "no-new-func": "error", | |
| "no-new-object": "error", | |
| "no-new-wrappers": "error", | |
| "no-octal-escape": "error", |
🤖 Prompt for AI Agents
In the .eslintrc.json file between lines 88 and 112, replace the deprecated
ESLint rule "no-native-reassign" with "no-global-assign" to avoid configuration
errors. Verify that the ESLint version used in CI supports the
"no-global-assign" rule to prevent linter failures. Remove "no-native-reassign"
entirely and add "no-global-assign" with the appropriate error level.
| cmd: | | ||
| ahoy cli "yarn install --frozen-lock" | ||
| #;< DRUPAL_THEME | ||
| ahoy cli "yarn --cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} install --frozen-lock" | ||
| #;> DRUPAL_THEME | ||
There was a problem hiding this comment.
Invalid Yarn flag --frozen-lock – should be --frozen-lockfile
yarn install exits with code 1 when an unknown flag is supplied, which will break ahoy fei locally and in CI.
-ahoy cli "yarn install --frozen-lock"
+ahoy cli "yarn install --frozen-lockfile"
…
-ahoy cli "yarn --cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} install --frozen-lock"
+ahoy cli "yarn --cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} install --frozen-lockfile"Please update both occurrences (and any in CI configs) to guarantee reproducible installs.
📝 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.
| cmd: | | |
| ahoy cli "yarn install --frozen-lock" | |
| #;< DRUPAL_THEME | |
| ahoy cli "yarn --cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} install --frozen-lock" | |
| #;> DRUPAL_THEME | |
| cmd: | | |
| ahoy cli "yarn install --frozen-lockfile" | |
| #;< DRUPAL_THEME | |
| ahoy cli "yarn --cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} install --frozen-lockfile" | |
| #;> DRUPAL_THEME |
🤖 Prompt for AI Agents
In the .ahoy.yml file around lines 167 to 172, the yarn install commands use the
invalid flag --frozen-lock instead of --frozen-lockfile. Update both yarn
install commands to replace --frozen-lock with --frozen-lockfile to ensure the
commands run correctly and guarantee reproducible installs locally and in CI.
AlexSkrypnyk
force-pushed
the
feature/remove-js-css-phpcs
branch
from
24fd912 to
e94e841
Compare
AlexSkrypnyk
force-pushed
the
feature/remove-js-css-phpcs
branch
from
e94e841 to
6a935c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
web/modules/custom/ys_base/ys_base.module (1)
12-26: Fix hook_theme signature and remove PHPStan ignore comment.The function signature should follow Drupal's standard hook_theme definition and the PHPStan ignore comment should be removed.
Apply this fix to align with Drupal standards:
-/** - * Implements hook_theme(). - * - * @phpstan-ignore-next-line - */ -function ys_base_theme(): array { +/** + * Implements hook_theme(). + * + * @param array $existing + * Existing theme implementations. + * @param string $type + * The type of hook. + * @param string $theme + * Theme name. + * @param string $path + * Module path. + * + * @return array + * Theme hook definitions. + */ +function ys_base_theme(array $existing, string $type, string $theme, string $path): array {web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php (1)
20-24: Remove unnecessary phpstan-ignore comment.The
@phpstan-ignore-next-linecomment appears unnecessary for thebuild()method override. This comment should only be used when there's a legitimate phpstan issue that cannot be resolved otherwise.- /** - * {@inheritdoc} - * - * @phpstan-ignore-next-line - */ + /** + * {@inheritdoc} + */web/modules/custom/ys_base/js/ys_base.js (3)
7-7: Remove redundant use strict directive.Modern JavaScript modules are automatically in strict mode.
- 'use strict';
36-36: Use Number.parseInt instead of global parseInt.ES2015 moved parseInt into the Number namespace for consistency.
- let currentValue = parseInt(localStorage.getItem(storageKey)) || 0; + let currentValue = Number.parseInt(localStorage.getItem(storageKey)) || 0;
69-69: Use template literals instead of string concatenation.Template literals are more readable and performant.
- console.log('Counter ' + action + ': ' + currentValue); + console.log(`Counter ${action}: ${currentValue}`);web/modules/custom/ys_base/css/ys_base.css (1)
11-11:max-width: 300pxis still hard-coded – breaks on narrow viewports
Same concern raised previously; consider a relative value or a media query for better responsiveness..eslintrc.json (1)
107-108:no-native-reassignhas been removed from ESLint – replace withno-global-assignThis rule was dropped in ESLint v4; leaving it causes “Configuration for rule not found” and exits with code 2.
- "no-native-reassign": "error", + "no-global-assign": "error",.ahoy.yml (1)
165-172: Theme-scopedyarn installstill uses invalid flag--frozen-lock
yarn installaborts on unknown flags, breakingahoy feilocally & in CI. Global install was fixed, but the theme install wasn’t.- ahoy cli "yarn --cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} install --frozen-lock" + ahoy cli "yarn --cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} install --frozen-lockfile"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (58)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.eslintignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.stylelintrc.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/phpcs.xmlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/tests/behat/features/counter.featureis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/css/sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/js/sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/src/Plugin/Block/CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/sw_base.moduleis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/templates/ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/ciprovider_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_all_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deps_updates_provider_ci_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/.eslintignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/css/sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/js/sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/src/Plugin/Block/CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/sw_base.moduleis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/templates/ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/-sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/css/-sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/js/-sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/src/Plugin/Block/-CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/templates/-ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/-sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/css/-sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/js/-sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/src/Plugin/Block/-CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/templates/-ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/css/the_force_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/js/the_force_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/src/Plugin/Block/CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/templates/ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/the_force_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/the_force_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/the_force_base.moduleis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_database_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_profile/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/web/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_custom/web/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
.ahoy.yml(4 hunks).circleci/config.yml(2 hunks).dockerignore(1 hunks).eslintignore(1 hunks).eslintrc.json(1 hunks).github/workflows/build-test-deploy.yml(2 hunks).stylelintrc.js(1 hunks).vortex/docs/.eslintrc.js(1 hunks).vortex/tests/bats/_helper.workflow.bash(3 hunks)package.json(1 hunks)phpcs.xml(1 hunks)tests/behat/features/counter.feature(1 hunks)web/modules/custom/ys_base/css/ys_base.css(1 hunks)web/modules/custom/ys_base/js/ys_base.js(1 hunks)web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php(1 hunks)web/modules/custom/ys_base/templates/ys-base-counter-block.html.twig(1 hunks)web/modules/custom/ys_base/ys_base.deploy.php(1 hunks)web/modules/custom/ys_base/ys_base.libraries.yml(1 hunks)web/modules/custom/ys_base/ys_base.module(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/behat/**/*.feature
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (8)
.eslintignore (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.circleci/config.yml (2)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Composer dependencies should be updated automatically with compatibility checks
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.dockerignore (1)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
package.json (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.github/workflows/build-test-deploy.yml (2)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Composer dependencies should be updated automatically with compatibility checks
tests/behat/features/counter.feature (3)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Applies to tests/behat/**/*.feature : Use consistent user types in Behat feature files: 'site visitor', 'site administrator', and 'content editor'
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Applies to tests/behat/**/*.feature : All Behat feature files must use the standard user story format: 'As a [user type]\nI want to [action]\nSo that [benefit]'
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Applies to tests/behat/**/*.feature : Do not use commas or periods in user story statements within Behat feature files
.ahoy.yml (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.vortex/tests/bats/_helper.workflow.bash (3)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#1693
File: .vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.yml:1-4
Timestamp: 2025-06-01T08:08:59.071Z
Learning: Test fixture files (typically located in directories containing "fixture" or "test") should not be reviewed for code quality issues as they may intentionally contain incomplete, invalid, or malformed data for testing purposes.
🪛 Biome (1.9.4)
web/modules/custom/ys_base/js/ys_base.js
[error] 7-7: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
[error] 69-69: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 36-36: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ 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). (2)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (0)
🔇 Additional comments (20)
.vortex/docs/.eslintrc.js (1)
2-2: Good practice: ESLint root configuration.Adding
root: truecorrectly establishes this as the root ESLint configuration, preventing conflicts from parent directory configurations. This is essential for maintaining isolated linting rules in the docs directory..eslintignore (1)
1-10: Comprehensive ESLint ignore configuration.The ignore patterns appropriately exclude third-party dependencies, Drupal core/contrib code, generated files, and minified assets. This focuses linting on custom code where it's most valuable.
.dockerignore (1)
32-34: Appropriate Docker build context updates.Adding
.eslintignoreand.stylelintrc.jsto the Docker build context ensures linting configurations are available in containerized environments, supporting the new frontend linting pipeline.phpcs.xml (1)
16-16: Correct separation of linting responsibilities.Removing "js" and "css" from PHPCS extensions is appropriate since dedicated tools (ESLint, Stylelint) are now handling frontend code linting. This eliminates overlap and ensures each tool focuses on its strengths.
web/modules/custom/ys_base/ys_base.libraries.yml (1)
1-9: LGTM! Proper Drupal library definition.The library definition follows Drupal conventions correctly with appropriate CSS categorization, JavaScript inclusion, and core dependency. The file structure and naming are consistent with Drupal standards.
.github/workflows/build-test-deploy.yml (4)
254-254: Good addition of yarn install for frontend dependencies.Adding
yarn install --frozen-lockfileensures consistent dependency installation in CI, which is a best practice for reproducible builds.
256-256: Improved step name clarity.The updated step name "Validate Composer configuration is normalized" is more grammatically correct and clearer than the previous version.
284-286: Well-structured separation of module and theme linting.The new step for linting module code with NodeJS linters provides clear separation between module and theme linting, improving CI organization and maintainability.
289-289: Improved step name for theme linting.The renamed step "Lint theme code with NodeJS linters" clearly distinguishes theme linting from the new module linting step, improving workflow clarity.
web/modules/custom/ys_base/ys_base.deploy.php (1)
17-46: Well-implemented deployment function.The function properly checks for existing blocks before creation, uses the correct Drupal entity API, and provides appropriate success/failure messages. The block configuration is comprehensive and follows Drupal standards.
package.json (1)
1-27: Excellent frontend tooling setup.The package.json establishes a comprehensive linting pipeline with appropriate dependencies, engine requirements, and well-structured scripts. The use of Airbnb ESLint config and Stylelint with standard configuration provides good code quality standards.
web/modules/custom/ys_base/templates/ys-base-counter-block.html.twig (1)
1-26: Well-structured and accessible template.The template follows good practices with proper documentation, semantic HTML structure, consistent class naming, and accessibility attributes. The use of data attributes for JavaScript targeting is appropriate, and the overall structure is clean and maintainable.
.stylelintrc.js (1)
1-39: Well-structured Stylelint configuration.The configuration appropriately extends the standard config, includes property ordering, and provides comprehensive Sass at-rule exceptions. The selective rule disabling appears pragmatic for a Drupal project context.
web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php (2)
25-35: Well-implemented block plugin.The render array structure correctly uses a theme hook, initializes the counter value, and attaches the necessary library for frontend functionality.
40-43: Appropriate caching strategy for interactive content.Disabling cache for this interactive counter block is the correct approach to ensure dynamic behavior works as expected.
.circleci/config.yml (3)
266-266: Good addition of yarn dependency installation.Adding
yarn install --frozen-lockfileafter Composer dependencies is the correct approach for managing frontend dependencies in the CI pipeline.
269-269: Improved step name clarity.The updated step name "Validate Composer configuration is normalized" is more grammatically correct and clear.
297-304: Well-structured separation of linting steps.Splitting NodeJS linting into separate steps for module code and theme code provides better granular control and clearer failure reporting in CI.
tests/behat/features/counter.feature (1)
8-39: Comprehensive test scenarios for counter functionality.The test scenarios effectively cover the counter block's core functionality including basic interactions, negative values, reset functionality, and persistence across page reloads.
web/modules/custom/ys_base/js/ys_base.js (1)
9-74: Well-implemented counter behavior with localStorage persistence.The JavaScript behavior correctly handles counter initialization, event handling, and localStorage persistence. The implementation follows Drupal JavaScript patterns and provides good user feedback.
| As a site visitor | ||
| I want to interact with the counter block | ||
| So that I can increment decrement and reset the counter value |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve user story readability while following guidelines.
The user story format is correct, but line 6 could be more readable. Consider revising to improve clarity without violating the no-comma rule.
- So that I can increment decrement and reset the counter value
+ So that I can increment or decrement and reset the counter value📝 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.
| As a site visitor | |
| I want to interact with the counter block | |
| So that I can increment decrement and reset the counter value | |
| As a site visitor | |
| I want to interact with the counter block | |
| So that I can increment or decrement and reset the counter value |
🤖 Prompt for AI Agents
In tests/behat/features/counter.feature around lines 4 to 6, the user story's
third line is hard to read due to lack of punctuation. Improve clarity by
rephrasing the line to separate the actions increment, decrement, and reset into
a clearer format without using commas, such as using conjunctions or breaking
into multiple lines.
| padding: 8px 12px; | ||
| transition: all 0.3s ease; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Transition all → restrict scoped properties
transition: all … animates every property and may introduce jank (e.g., width, box-shadow). Limit it to the properties that actually change:
- transition: all 0.3s ease;
+ transition: background-color 0.3s ease, transform 0.3s ease;📝 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.
| padding: 8px 12px; | |
| transition: all 0.3s ease; | |
| } | |
| padding: 8px 12px; | |
| - transition: all 0.3s ease; | |
| + transition: background-color 0.3s ease, transform 0.3s ease; | |
| } |
🤖 Prompt for AI Agents
In web/modules/custom/ys_base/css/ys_base.css around lines 34 to 36, the CSS
uses 'transition: all 0.3s ease;' which animates all properties and can cause
performance issues. Replace 'all' with a comma-separated list of only the CSS
properties that actually change during the transition, such as
'background-color', 'color', or 'opacity', to restrict the scope and improve
performance.
| echo ".abc{margin: 0px;}" >>"${webroot}/modules/custom/sw_base/css/sw_base.css" | ||
| sync_to_container | ||
| run ahoy lint-fe | ||
| assert_failure | ||
| rm -f "${webroot}/modules/custom/sw_base/css/sw_base.css" | ||
| ahoy cli rm -f "${webroot}/modules/custom/sw_base/css/sw_base.css" | ||
| sync_to_container | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid deleting real source – use restore_file instead of rm -f
rm -f "${webroot}/modules/custom/sw_base/css/sw_base.css" obliterates the module stylesheet if it existed before the test.
Safer pattern:
-rm -f "${webroot}/modules/custom/sw_base/css/sw_base.css"
+restore_file "${webroot}/modules/custom/sw_base/css/sw_base.css"Keeps original contents intact and aligns with the restore strategy used elsewhere in the script.
📝 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.
| echo ".abc{margin: 0px;}" >>"${webroot}/modules/custom/sw_base/css/sw_base.css" | |
| sync_to_container | |
| run ahoy lint-fe | |
| assert_failure | |
| rm -f "${webroot}/modules/custom/sw_base/css/sw_base.css" | |
| ahoy cli rm -f "${webroot}/modules/custom/sw_base/css/sw_base.css" | |
| sync_to_container | |
| echo ".abc{margin: 0px;}" >>"${webroot}/modules/custom/sw_base/css/sw_base.css" | |
| sync_to_container | |
| run ahoy lint-fe | |
| assert_failure | |
| - rm -f "${webroot}/modules/custom/sw_base/css/sw_base.css" | |
| + restore_file "${webroot}/modules/custom/sw_base/css/sw_base.css" | |
| ahoy cli rm -f "${webroot}/modules/custom/sw_base/css/sw_base.css" | |
| sync_to_container |
🤖 Prompt for AI Agents
In .vortex/tests/bats/_helper.workflow.bash around lines 458 to 465, replace the
direct deletion of the real source file using rm -f with the restore_file
function to safely revert the sw_base.css file to its original state after the
test. This prevents permanent loss of the original stylesheet and maintains
consistency with the existing restore strategy in the script.
| cp "${webroot}/modules/custom/sw_base/js/sw_base.js" "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" | ||
| echo "console.log('abc;" >>"${webroot}/modules/custom/sw_base/js/sw_base.js" | ||
| sync_to_container | ||
| run ahoy lint-fe | ||
| assert_failure | ||
| rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js" | ||
| cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js" | ||
| ahoy cli rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js" | ||
| ahoy cli cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js" | ||
| rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" | ||
| ahoy cli rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" | ||
| sync_to_container |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Redundant in-container copy/remove cycles – single sync_to_container is enough
The sequence ahoy cli rm … → ahoy cli cp … → final sync_to_container performs the same work twice and prolongs test time.
Consider:
- ahoy cli rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js"
- ahoy cli cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js"
- ...
- sync_to_container
+ restore_file "${webroot}/modules/custom/sw_base/js/sw_base.js"
+ rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak"
+ sync_to_containerSimplifies the flow and re-uses helper utilities.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .vortex/tests/bats/_helper.workflow.bash around lines 470 to 481, the code
redundantly removes and copies files inside the container using `ahoy cli rm`
and `ahoy cli cp` commands, followed by a final `sync_to_container`. To fix
this, remove the in-container `ahoy cli rm` and `ahoy cli cp` commands and rely
on a single `sync_to_container` call after restoring the original file locally.
This simplifies the flow and avoids unnecessary duplicate file operations.
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
.github/workflows/build-test-deploy.yml (1)
249-255: Cache the Yarn install to avoid re-fetching node_modules on every build
yarn install --frozen-lockfileis executed for every CI matrix instance but the workflow does not restore/save a cache for the Yarn artefacts (~/.cache/yarnandnode_modules).
Caching these directories typically drops several minutes from the pipeline and reduces network flakiness.- name: Install development dependencies run: | + # Restore Yarn cache + if test -d ~/.cache/yarn; then echo "Yarn cache restored"; fi docker compose exec $(env | cut -f1 -d= | sed 's/^/-e /') -T cli bash -c " \ if [ -n \"${GITHUB_TOKEN:-}\" ]; then export COMPOSER_AUTH='{\"github-oauth\": {\"github.com\": \"${GITHUB_TOKEN-}\"}}'; fi && \ COMPOSER_MEMORY_LIMIT=-1 composer --ansi install --prefer-dist" docker compose exec $(env | cut -f1 -d= | sed 's/^/-e /') -T cli bash -c "yarn install --frozen-lockfile" + + - name: Save Yarn cache + uses: actions/cache@v4 + with: + path: | + ~/.cache/yarn + node_modules + key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }} + restore-keys: | + ${{ runner.os }}-yarn-
♻️ Duplicate comments (7)
web/modules/custom/ys_base/ys_base.module (1)
15-15: The PHPStan ignore issue persists.This is the same issue identified in the previous review - the function signature doesn't match Drupal's standard hook_theme parameters, requiring the PHPStan ignore.
web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php (1)
23-23: Remove unnecessary phpstan-ignore comment.The
@phpstan-ignore-next-linecomment is unnecessary for the standardbuild()method override.web/modules/custom/ys_base/js/ys_base.js (3)
7-7: Remove redundant use strict directive.Modern JavaScript modules are automatically in strict mode, making this directive unnecessary.
36-36: Use Number.parseInt instead of global parseInt.ES2015 moved parseInt into the Number namespace for consistency.
69-69: Use template literals instead of string concatenation.Template literals are more readable and performant than string concatenation.
web/modules/custom/ys_base/css/ys_base.css (1)
11-11: Fixed width still harms responsiveness – please switch to relative units or add media queries
We previously flagged the hard-codedmax-width: 300px;. Nothing changed – it will still overflow on narrow viewports..ahoy.yml (1)
168-172:--frozen-lockis not a valid Yarn flag – it must be--frozen-lockfile
The second install still uses the invalid flag, soahoy feiwill fail in CI and locally.- ahoy cli "yarn --cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} install --frozen-lock" + ahoy cli "yarn --cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} install --frozen-lockfile"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (58)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.eslintignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.stylelintrc.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/phpcs.xmlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/tests/behat/features/counter.featureis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/css/sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/js/sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/src/Plugin/Block/CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/sw_base.moduleis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/templates/ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/ciprovider_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_all_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deps_updates_provider_ci_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/.eslintignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/css/sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/js/sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/src/Plugin/Block/CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/sw_base.moduleis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/templates/ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/-sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/css/-sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/js/-sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/src/Plugin/Block/-CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/templates/-ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/-sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/css/-sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/js/-sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/src/Plugin/Block/-CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/templates/-ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/css/the_force_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/js/the_force_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/src/Plugin/Block/CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/templates/ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/the_force_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/the_force_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/the_force_base.moduleis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_database_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_profile/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/web/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_custom/web/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
.ahoy.yml(4 hunks).circleci/config.yml(2 hunks).dockerignore(1 hunks).eslintignore(1 hunks).eslintrc.json(1 hunks).github/workflows/build-test-deploy.yml(2 hunks).stylelintrc.js(1 hunks).vortex/docs/.eslintrc.js(1 hunks).vortex/tests/bats/_helper.workflow.bash(3 hunks)package.json(1 hunks)phpcs.xml(1 hunks)tests/behat/features/counter.feature(1 hunks)web/modules/custom/ys_base/css/ys_base.css(1 hunks)web/modules/custom/ys_base/js/ys_base.js(1 hunks)web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php(1 hunks)web/modules/custom/ys_base/templates/ys-base-counter-block.html.twig(1 hunks)web/modules/custom/ys_base/ys_base.deploy.php(1 hunks)web/modules/custom/ys_base/ys_base.libraries.yml(1 hunks)web/modules/custom/ys_base/ys_base.module(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/behat/**/*.feature
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (8)
.dockerignore (1)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
.eslintignore (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.github/workflows/build-test-deploy.yml (2)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Composer dependencies should be updated automatically with compatibility checks
package.json (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.circleci/config.yml (2)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Composer dependencies should be updated automatically with compatibility checks
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.vortex/tests/bats/_helper.workflow.bash (4)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#1693
File: .vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.yml:1-4
Timestamp: 2025-06-01T08:08:59.071Z
Learning: Test fixture files (typically located in directories containing "fixture" or "test") should not be reviewed for code quality issues as they may intentionally contain incomplete, invalid, or malformed data for testing purposes.
.ahoy.yml (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
tests/behat/features/counter.feature (3)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Applies to tests/behat/**/*.feature : Use consistent user types in Behat feature files: 'site visitor', 'site administrator', and 'content editor'
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Applies to tests/behat/**/*.feature : All Behat feature files must use the standard user story format: 'As a [user type]\nI want to [action]\nSo that [benefit]'
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Applies to tests/behat/**/*.feature : Do not use commas or periods in user story statements within Behat feature files
🪛 Biome (1.9.4)
web/modules/custom/ys_base/js/ys_base.js
[error] 7-7: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
[error] 69-69: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 36-36: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ 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). (2)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (0)
🔇 Additional comments (17)
web/modules/custom/ys_base/ys_base.module (1)
17-26: LGTM! Theme hook registration is well-structured.The theme hook implementation correctly registers the counter block template with appropriate default variables and template naming conventions.
.vortex/docs/.eslintrc.js (1)
2-2: LGTM! Proper ESLint configuration isolation.Adding
root: trueensures this directory's ESLint configuration is self-contained and doesn't inherit from parent directories, which is the correct approach for isolated documentation linting.web/modules/custom/ys_base/ys_base.libraries.yml (1)
1-10: Referenced CSS and JS files verifiedBoth
web/modules/custom/ys_base/css/ys_base.cssandweb/modules/custom/ys_base/js/ys_base.jsexist as expected. No further action required..dockerignore (1)
32-32: LGTM! Proper inclusion of new linting configuration files.Adding
.eslintignoreand.stylelintrc.jsto the Docker build context supports the new front-end linting infrastructure introduced in this PR.Also applies to: 34-34
phpcs.xml (1)
16-16: LGTM! Proper separation of linting responsibilities.Removing JS and CSS extensions from PHPCS allows specialized tools (ESLint/Stylelint) to handle front-end code linting while PHPCS focuses on PHP code quality.
.github/workflows/build-test-deploy.yml (1)
284-287: Make the module-level lint step fail fast on missing script
yarn run lintwill exit with code 1 if the script is missing, but will exit with code 0 if the script exists and has no files to lint even when rules are not applied because of a mis-configuration.
Consider adding--verboseor enablingeslint --print-configin the lint script to surface mis-configurations early.package.json (1)
18-26: Verify Stylelint / config version compatibility
stylelint@16moved to ESM-only.stylelint-config-standard@36is indeed compatible, but older CI runners occasionally cache Node 14/16 causing “Cannot use import statement outside a module” errors.
Please double-check that the Docker imagedrevops/ci-runner:25.7.0ships with Node ≥ 20 as required by theenginesfield..eslintignore (1)
1-10: Looks good – sensible defaultsThe ignore list covers vendor, contrib and minified assets without excluding custom source files.
.stylelintrc.js (1)
1-39: LGTM! Well-configured Stylelint setup for the project.The configuration appropriately extends the standard Stylelint rules, enables property ordering for consistency, and allows necessary Sass at-rules while disabling overly strict pattern rules. This provides a good balance between code quality enforcement and developer flexibility.
web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php (2)
25-35: Well-structured block implementation.The render array properly uses a theme hook, initializes the counter value, and attaches the required JavaScript library. Good separation of concerns.
40-43: Appropriate cache configuration for interactive content.Disabling caching (
return 0) is correct for this interactive block to ensure proper functionality.tests/behat/features/counter.feature (2)
4-6: Proper user story format following Behat guidelines.The user story correctly uses the standard format with consistent user type "site visitor" and follows the guideline to avoid commas/periods in statements.
8-40: Comprehensive test coverage for counter functionality.The scenarios thoroughly test counter visibility, increment/decrement actions, reset functionality, and persistence across page reloads. Good use of JavaScript tags for interactive testing.
.circleci/config.yml (3)
266-266: Good addition of yarn install step.Adding
yarn install --frozen-lockfileafter Composer dependencies ensures Node.js dependencies are available for the linting steps.
269-269: Improved step naming for clarity.The renamed step "Validate Composer configuration is normalized" is more grammatically correct and clear.
297-304: Well-structured separation of module and theme linting.Splitting Node.js linting into separate steps for module and theme code provides better granularity and clearer error reporting while maintaining the existing failure handling logic.
web/modules/custom/ys_base/js/ys_base.js (1)
20-73: Well-implemented counter functionality.The counter logic is solid with proper event handling, localStorage persistence, visual feedback, and duplicate processing prevention. Good separation of concerns and clear code structure.
| // Create block configuration. | ||
| $block_config = [ | ||
| 'id' => 'ys_base_counter_block', | ||
| 'theme' => \Drupal::config('system.theme')->get('default'), | ||
| 'region' => 'content', | ||
| 'weight' => 10, | ||
| 'plugin' => 'ys_base_counter_block', | ||
| 'settings' => [ | ||
| 'id' => 'ys_base_counter_block', | ||
| 'label' => 'Counter Block', | ||
| 'label_display' => 'visible', | ||
| 'provider' => 'ys_base', | ||
| ], | ||
| ]; |
There was a problem hiding this comment.
Block configuration: move provider & label outside settings
In a block config entity the top-level keys should include provider and status; settings must contain only plugin-specific options.
Placing provider inside settings produces extra, unknown config that fails strict config-schema validation and may be silently ignored.
$block_config = [
'id' => 'ys_base_counter_block',
'theme' => \Drupal::config('system.theme')->get('default'),
'region' => 'content',
'weight' => 10,
'plugin' => 'ys_base_counter_block',
- 'settings' => [
- 'id' => 'ys_base_counter_block',
- 'label' => 'Counter Block',
- 'label_display' => 'visible',
- 'provider' => 'ys_base',
- ],
+ 'label' => 'Counter Block',
+ 'settings' => [
+ 'id' => 'ys_base_counter_block',
+ 'label' => 'Counter Block',
+ 'label_display' => 'visible',
+ ],
+ 'provider' => 'ys_base',
+ 'status' => 1,
];Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In web/modules/custom/ys_base/ys_base.deploy.php around lines 26 to 39, move the
'provider' and 'label' keys out of the 'settings' array to the top-level of the
$block_config array. Ensure 'provider' is a top-level key alongside 'id',
'theme', 'region', 'weight', and 'plugin', and 'label' is also moved out if it
is not plugin-specific. Keep only plugin-specific options inside 'settings' to
comply with Drupal's block config schema and avoid validation errors.
| <div class="ys-base-counter-display"> | ||
| <span class="ys-base-counter-label">Counter:</span> | ||
| <span class="ys-base-counter-value" data-counter-value>{{ counter_value }}</span> | ||
| </div> | ||
| <div class="ys-base-counter-controls"> | ||
| <button class="ys-base-counter-btn ys-base-counter-btn--decrement" data-counter-action="decrement" aria-label="Decrease counter"> | ||
| <span class="ys-base-counter-btn-text">-</span> | ||
| </button> | ||
| <button class="ys-base-counter-btn ys-base-counter-btn--increment" data-counter-action="increment" aria-label="Increase counter"> | ||
| <span class="ys-base-counter-btn-text">+</span> | ||
| </button> | ||
| </div> | ||
| <div class="ys-base-counter-actions"> | ||
| <button class="ys-base-counter-btn ys-base-counter-btn--reset" data-counter-action="reset">Reset</button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mark user-visible strings for translation
Hard-coded English strings (Counter:, button texts and ARIA labels) are not passed through Drupal’s translation filters, so the block will always render in English.
- <span class="ys-base-counter-label">Counter:</span>
+ <span class="ys-base-counter-label">{{ 'Counter:'|t }}</span>
- <button ... aria-label="Decrease counter">
+ <button ... aria-label="{{ 'Decrease counter'|t }}">
- <button ... aria-label="Increase counter">
+ <button ... aria-label="{{ 'Increase counter'|t }}">
- <button class="ys-base-counter-btn ys-base-counter-btn--reset" data-counter-action="reset">Reset</button>
+ <button class="ys-base-counter-btn ys-base-counter-btn--reset" data-counter-action="reset">{{ 'Reset'|t }}</button>📝 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.
| <div class="ys-base-counter-display"> | |
| <span class="ys-base-counter-label">Counter:</span> | |
| <span class="ys-base-counter-value" data-counter-value>{{ counter_value }}</span> | |
| </div> | |
| <div class="ys-base-counter-controls"> | |
| <button class="ys-base-counter-btn ys-base-counter-btn--decrement" data-counter-action="decrement" aria-label="Decrease counter"> | |
| <span class="ys-base-counter-btn-text">-</span> | |
| </button> | |
| <button class="ys-base-counter-btn ys-base-counter-btn--increment" data-counter-action="increment" aria-label="Increase counter"> | |
| <span class="ys-base-counter-btn-text">+</span> | |
| </button> | |
| </div> | |
| <div class="ys-base-counter-actions"> | |
| <button class="ys-base-counter-btn ys-base-counter-btn--reset" data-counter-action="reset">Reset</button> | |
| </div> | |
| <div class="ys-base-counter-display"> | |
| <span class="ys-base-counter-label">{{ 'Counter:'|t }}</span> | |
| <span class="ys-base-counter-value" data-counter-value>{{ counter_value }}</span> | |
| </div> | |
| <div class="ys-base-counter-controls"> | |
| <button class="ys-base-counter-btn ys-base-counter-btn--decrement" | |
| data-counter-action="decrement" | |
| aria-label="{{ 'Decrease counter'|t }}"> | |
| <span class="ys-base-counter-btn-text">-</span> | |
| </button> | |
| <button class="ys-base-counter-btn ys-base-counter-btn--increment" | |
| data-counter-action="increment" | |
| aria-label="{{ 'Increase counter'|t }}"> | |
| <span class="ys-base-counter-btn-text">+</span> | |
| </button> | |
| </div> | |
| <div class="ys-base-counter-actions"> | |
| <button class="ys-base-counter-btn ys-base-counter-btn--reset" | |
| data-counter-action="reset"> | |
| {{ 'Reset'|t }} | |
| </button> | |
| </div> |
🤖 Prompt for AI Agents
In web/modules/custom/ys_base/templates/ys-base-counter-block.html.twig lines 11
to 25, the user-visible strings such as "Counter:", button texts "-", "+",
"Reset", and ARIA labels are hard-coded and not wrapped in Drupal's translation
filter. To fix this, wrap all these strings with the Twig translation filter
(e.g., {{ 'Counter:'|t }}) to enable localization and ensure the block renders
correctly in different languages.
| transition: all 0.3s ease; | ||
| } | ||
|
|
||
| .ys-base-counter-value.updated { | ||
| background: #28a745; | ||
| transform: scale(1.1); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid transition: all for performance
Transitioning every property forces the browser to track far more than necessary and can trigger layout / paint on each update. Restrict the transition to the properties that actually change (e.g. background-color, transform).
- transition: all 0.3s ease;
+ transition: background-color 0.3s ease, transform 0.3s ease;📝 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.
| transition: all 0.3s ease; | |
| } | |
| .ys-base-counter-value.updated { | |
| background: #28a745; | |
| transform: scale(1.1); | |
| } | |
| transition: background-color 0.3s ease, transform 0.3s ease; | |
| } | |
| .ys-base-counter-value.updated { | |
| background: #28a745; | |
| transform: scale(1.1); | |
| } |
🤖 Prompt for AI Agents
In web/modules/custom/ys_base/css/ys_base.css around lines 35 to 41, replace the
generic 'transition: all 0.3s ease;' with a more specific transition targeting
only the properties that change, such as 'background-color' and 'transform'.
This will improve performance by limiting the browser's rendering work to just
those properties.
| echo ".abc{margin: 0px;}" >>"${webroot}/modules/custom/sw_base/css/sw_base.css" | ||
| sync_to_container | ||
| run ahoy lint-fe | ||
| assert_failure | ||
| rm -f "${webroot}/modules/custom/sw_base/css/sw_base.css" | ||
| ahoy cli rm -f "${webroot}/modules/custom/sw_base/css/sw_base.css" | ||
| sync_to_container | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Host + container deletions duplicate effort
You already call sync_to_container right after the host-side rm. Removing the same file inside the container (ahoy cli rm -f …) is redundant. Dropping the extra command shortens the test by ~1 s per run with no loss of coverage.
🤖 Prompt for AI Agents
In .vortex/tests/bats/_helper.workflow.bash around lines 458 to 465, the file
deletion is done twice: once on the host with rm and once inside the container
with ahoy cli rm. Since sync_to_container is called right after the host rm, the
container deletion is redundant. Remove the line with ahoy cli rm -f to
eliminate duplicate deletion and improve test runtime without losing coverage.
| cp "${webroot}/modules/custom/sw_base/js/sw_base.js" "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" | ||
| echo "console.log('abc;" >>"${webroot}/modules/custom/sw_base/js/sw_base.js" | ||
| sync_to_container | ||
| run ahoy lint-fe | ||
| assert_failure | ||
| rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js" | ||
| cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js" | ||
| ahoy cli rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js" | ||
| ahoy cli cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js" | ||
| rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" | ||
| ahoy cli rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complex restore logic – race-prone and unnecessary container copies
The restore path:
rm …cp …ahoy cli rm …ahoy cli cp …rm …bakahoy cli rm …baksync_to_container
Only steps 1-2 and the final sync_to_container are required. The in-container cp/rm pairs fight with the subsequent sync and can fail if the relative path differs inside the container (/app/web/…).
- rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js"
- cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js"
- ahoy cli rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js"
- ahoy cli cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js"
- rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak"
- ahoy cli rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak"
+ rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js"
+ cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js"
+ rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak"sync_to_container will propagate the changes in one go.
📝 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.
| cp "${webroot}/modules/custom/sw_base/js/sw_base.js" "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" | |
| echo "console.log('abc;" >>"${webroot}/modules/custom/sw_base/js/sw_base.js" | |
| sync_to_container | |
| run ahoy lint-fe | |
| assert_failure | |
| rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js" | |
| cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js" | |
| ahoy cli rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js" | |
| ahoy cli cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js" | |
| rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" | |
| ahoy cli rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" | |
| cp "${webroot}/modules/custom/sw_base/js/sw_base.js" "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" | |
| echo "console.log('abc;" >>"${webroot}/modules/custom/sw_base/js/sw_base.js" | |
| sync_to_container | |
| run ahoy lint-fe | |
| assert_failure | |
| - rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js" | |
| - cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js" | |
| - ahoy cli rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js" | |
| - ahoy cli cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js" | |
| - rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" | |
| - ahoy cli rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" | |
| + rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js" | |
| + cp "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" "${webroot}/modules/custom/sw_base/js/sw_base.js" | |
| + rm -f "${webroot}/modules/custom/sw_base/js/sw_base.js.bak" |
🤖 Prompt for AI Agents
In .vortex/tests/bats/_helper.workflow.bash around lines 470 to 480, simplify
the restore logic by removing the redundant in-container file operations (ahoy
cli rm and cp commands) and extra local file removals. Keep only the local rm
and cp commands to restore the backup, then call sync_to_container once at the
end to propagate changes into the container. This avoids race conditions and
unnecessary container copies.
| "no-caller": "error", | ||
| "no-catch-shadow": "error", | ||
| "no-eval": "error", | ||
| "no-extend-native": "error", | ||
| "no-extra-bind": "error", | ||
| "no-extra-parens": [ | ||
| "error", | ||
| "functions" | ||
| ], | ||
| "no-implied-eval": "error", | ||
| "no-iterator": "error", | ||
| "no-label-var": "error", | ||
| "no-labels": "error", | ||
| "no-lone-blocks": "error", | ||
| "no-loop-func": "error", | ||
| "no-multi-spaces": "error", | ||
| "no-multi-str": "error", | ||
| "no-native-reassign": "error", | ||
| "no-nested-ternary": "error", | ||
| "no-new-func": "error", |
There was a problem hiding this comment.
Deprecated ESLint rules will break the linter run
no-native-reassign, no-catch-shadow, and no-spaced-func were removed in ESLint ≥ v4 / v3. Keeping them yields “Configuration for rule ‹…› not found” and exits with code 2.
- "no-catch-shadow": "error",
…
- "no-native-reassign": "error",
…
- "no-spaced-func": "error",
+ "no-shadow": ["error", { "builtinGlobals": false, "hoist": "functions" }],
+ "no-global-assign": "error",
+ "func-call-spacing": ["error", "never"],Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In the .eslintrc.json file between lines 90 and 109, the rules
"no-native-reassign", "no-catch-shadow", and "no-spaced-func" are deprecated and
cause the linter to fail. Remove these deprecated rules from the configuration
to prevent the "Configuration for rule ‹…› not found" error and allow the linter
to run successfully.
| ahoy cli vendor/bin/twig-cs-fixer lint | ||
| ahoy cli "yarn run lint" | ||
| #;< DRUPAL_THEME | ||
| ahoy cli "yarn run --cwd=\${WEBROOT}/themes/custom/\${DRUPAL_THEME} lint" | ||
| #;> DRUPAL_THEME | ||
| #;> DRUPAL_THEME | ||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Run theme lint only when theme exists
If DRUPAL_THEME is unset the root yarn run lint already covers all JS/CSS. Guard the theme-scoped lint with a [ -n "${DRUPAL_THEME}" ] check to avoid “No such file or directory” errors in projects without a custom theme.
🤖 Prompt for AI Agents
In the .ahoy.yml file around lines 204 to 209, the theme-specific lint command
runs unconditionally, causing errors if DRUPAL_THEME is unset. Wrap the theme
lint command in a conditional check that runs it only if DRUPAL_THEME is set and
non-empty by using a shell test like [ -n "${DRUPAL_THEME}" ] before executing
the theme-scoped lint command.
…S and JS linting.
AlexSkrypnyk
force-pushed
the
feature/remove-js-css-phpcs
branch
from
6a935c1 to
e5cee5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
.vortex/tests/bats/_helper.workflow.bash (1)
453-510: Consider extracting a helper for “break-lint-→ assert fail → cleanup” flow
The four FE-lint blocks repeat identical logic (create bad file, sync, expect fail, cleanup, sync, expect pass). A small shell function (e.g.lint_expect_failure <path> <bad_content>) would DRY this section and shorten future maintenance.
♻️ Duplicate comments (13)
web/modules/custom/ys_base/ys_base.module (1)
15-17: Fix function signature and remove PHPStan ignore comment.The function signature doesn't match Drupal's standard hook_theme definition, which requires four parameters. The PHPStan ignore comment is masking this issue instead of fixing it.
Update the function signature to match the standard hook_theme definition:
-/** - * Implements hook_theme(). - * - * @phpstan-ignore-next-line - */ -function ys_base_theme(): array { +/** + * Implements hook_theme(). + * + * @param array $existing + * Existing theme implementations. + * @param string $type + * The type of hook. + * @param string $theme + * Theme name. + * @param string $path + * Module path. + * + * @return array + * Theme hook definitions. + */ +function ys_base_theme(array $existing, string $type, string $theme, string $path): array {web/modules/custom/ys_base/ys_base.deploy.php (1)
27-39:provider&labelmust be top-level, not insidesettings
Configuration outside the schema will be silently dropped and can break config-export.'plugin' => 'ys_base_counter_block', - 'settings' => [ - 'id' => 'ys_base_counter_block', - 'label' => 'Counter Block', - 'label_display' => 'visible', - 'provider' => 'ys_base', - ], + 'label' => 'Counter Block', + 'provider' => 'ys_base', + 'status' => 1, + 'settings' => [ + 'label_display' => 'visible', + ],web/modules/custom/ys_base/templates/ys-base-counter-block.html.twig (1)
12-25: Mark UI strings for translationHard-coded English strings (
Counter:,Reset, ARIA labels, etc.) bypass Drupal’s i18n system.- <span class="ys-base-counter-label">Counter:</span> + <span class="ys-base-counter-label">{{ 'Counter:'|t }}</span> … - <button … aria-label="Decrease counter"> + <button … aria-label="{{ 'Decrease counter'|t }}"> … - <button … aria-label="Increase counter"> + <button … aria-label="{{ 'Increase counter'|t }}"> … - <button class="ys-base-counter-btn …" data-counter-action="reset">Reset</button> + <button class="ys-base-counter-btn …" data-counter-action="reset">{{ 'Reset'|t }}</button>web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php (1)
20-24: Remove unnecessary phpstan-ignore comment.The
@phpstan-ignore-next-linecomment appears unnecessary for thebuild()method override. This comment should only be used when there's a legitimate phpstan issue that cannot be resolved otherwise.- /** - * {@inheritdoc} - * - * @phpstan-ignore-next-line - */ + /** + * {@inheritdoc} + */tests/behat/features/counter.feature (1)
4-6: Improve user story readability while following guidelines.The user story format is correct, but line 6 could be more readable. Consider revising to improve clarity without violating the no-comma rule.
- So that I can increment decrement and reset the counter value + So that I can increment or decrement and reset the counter valueweb/modules/custom/ys_base/js/ys_base.js (3)
7-7: Remove redundant use strict directive.Modern JavaScript modules are automatically in strict mode.
- 'use strict';
36-36: Use Number.parseInt instead of global parseInt.ES2015 moved parseInt into the Number namespace for consistency.
- let currentValue = parseInt(localStorage.getItem(storageKey)) || 0; + let currentValue = Number.parseInt(localStorage.getItem(storageKey)) || 0;
69-69: Use template literals instead of string concatenation.Template literals are more readable and performant.
- console.log('Counter ' + action + ': ' + currentValue); + console.log(`Counter ${action}: ${currentValue}`);web/modules/custom/ys_base/css/ys_base.css (2)
11-12: Fixedmax-widthmay break on small viewports – switch to responsive units
Using an absolute300pxcap risks overflow on ≤320 px devices. Prefermax-width: min(300px,90vw);or a media-query break-point.
35-36:transition: allhurts performance; restrict to the animated props
Target the exact properties (background-color,transform) instead of animating every style..vortex/tests/bats/_helper.workflow.bash (1)
458-476: Redundant in-containerrm– host-side delete +sync_to_containeris enough
The patternrm -f …/sw_base.test.css ahoy cli rm -f …/sw_base.test.css sync_to_containerduplicates work and lengthens the suite. Drop the
ahoy cli rm -fstep;sync_to_containerwill propagate the host change. Same for the JS section a few lines below..eslintrc.json (1)
90-119: Remove deprecated ESLint rules – linter will error out
no-catch-shadow,no-native-reassign, andno-spaced-funcwere removed years ago. Keeping them yields “Configuration for rule … not found” (exit 2).- "no-catch-shadow": "error", … - "no-native-reassign": "error", … - "no-spaced-func": "error", + "no-shadow": ["error", { "builtinGlobals": false, "hoist": "functions" }], + "no-global-assign": "error", + "func-call-spacing": ["error", "never"],.ahoy.yml (1)
168-171: Invalid Yarn flag--frozen-lock– should be--frozen-lockfile
The shorter variant isn’t recognised and exits with code 1. Update the theme-scoped install:-ahoy cli "yarn --cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} install --frozen-lock" +ahoy cli "yarn --cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} install --frozen-lockfile"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (58)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.eslintignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.eslintrc.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.stylelintrc.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/phpcs.xmlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/tests/behat/features/counter.featureis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/css/sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/js/sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/src/Plugin/Block/CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/sw_base.moduleis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/modules/custom/sw_base/templates/ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/ciprovider_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_all_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deps_updates_provider_ci_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/.eslintignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/css/sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/js/sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/src/Plugin/Block/CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/sw_base.moduleis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/docroot/modules/custom/sw_base/templates/ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/-sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/css/-sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/js/-sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/src/Plugin/Block/-CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_acquia/web/modules/custom/sw_base/templates/-ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/hosting_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/-sw_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/css/-sw_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/js/-sw_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/src/Plugin/Block/-CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/sw_base/templates/-ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/css/the_force_base.cssis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/js/the_force_base.jsis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/src/Plugin/Block/CounterBlock.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/templates/ys-base-counter-block.html.twigis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/the_force_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/the_force_base.libraries.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/web/modules/custom/the_force_base/the_force_base.moduleis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_database_lagoon/.dockerignoreis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_profile/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/web/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_custom/web/modules/custom/sw_base/sw_base.deploy.phpis excluded by!.vortex/installer/tests/Fixtures/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
.ahoy.yml(4 hunks).circleci/config.yml(2 hunks).dockerignore(1 hunks).eslintignore(1 hunks).eslintrc.json(1 hunks).github/workflows/build-test-deploy.yml(2 hunks).stylelintrc.js(1 hunks).vortex/docs/.eslintrc.js(1 hunks).vortex/tests/bats/_helper.workflow.bash(3 hunks)package.json(1 hunks)phpcs.xml(1 hunks)tests/behat/features/counter.feature(1 hunks)web/modules/custom/ys_base/css/ys_base.css(1 hunks)web/modules/custom/ys_base/js/ys_base.js(1 hunks)web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php(1 hunks)web/modules/custom/ys_base/templates/ys-base-counter-block.html.twig(1 hunks)web/modules/custom/ys_base/ys_base.deploy.php(1 hunks)web/modules/custom/ys_base/ys_base.libraries.yml(1 hunks)web/modules/custom/ys_base/ys_base.module(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/behat/**/*.feature
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (8)
.eslintignore (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.dockerignore (1)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
tests/behat/features/counter.feature (3)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Applies to tests/behat/**/*.feature : Use consistent user types in Behat feature files: 'site visitor', 'site administrator', and 'content editor'
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Applies to tests/behat/**/*.feature : All Behat feature files must use the standard user story format: 'As a [user type]\nI want to [action]\nSo that [benefit]'
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Applies to tests/behat/**/*.feature : Do not use commas or periods in user story statements within Behat feature files
.github/workflows/build-test-deploy.yml (2)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Composer dependencies should be updated automatically with compatibility checks
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.circleci/config.yml (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Composer dependencies should be updated automatically with compatibility checks
.vortex/tests/bats/_helper.workflow.bash (3)
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#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.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#1693
File: .vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.yml:1-4
Timestamp: 2025-06-01T08:08:59.071Z
Learning: Test fixture files (typically located in directories containing "fixture" or "test") should not be reviewed for code quality issues as they may intentionally contain incomplete, invalid, or malformed data for testing purposes.
package.json (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
.ahoy.yml (1)
Learnt from: CR
PR: drevops/vortex#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-12T00:26:22.700Z
Learning: Node.js dependencies should be updated in theme directories
🪛 Biome (1.9.4)
web/modules/custom/ys_base/js/ys_base.js
[error] 7-7: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
[error] 69-69: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 36-36: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ 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). (13)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (2)
- 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)
- GitHub Check: vortex-test-docs
🔇 Additional comments (14)
.vortex/docs/.eslintrc.js (1)
2-2: Good addition of root configuration directive.Adding
root: trueis a best practice that prevents ESLint from searching parent directories for additional configurations, ensuring this config is definitive for the docs directory and improving linting performance.web/modules/custom/ys_base/ys_base.module (1)
18-25: Theme hook implementation is well-structured.The theme hook definition correctly:
- Maps to the appropriate template name
- Provides a sensible default variable
- Follows Drupal theming conventions
.dockerignore (1)
32-34: Appropriate addition of linting configuration files.Including
.eslintignoreand.stylelintrc.jsin the Docker build context is correct, as these files are needed for the enhanced frontend linting workflows implemented in this PR.web/modules/custom/ys_base/ys_base.libraries.yml (1)
1-10: Library assets verified and library definition is valid.
- Confirmed existence of web/modules/custom/ys_base/css/ys_base.css
- Confirmed existence of web/modules/custom/ys_base/js/ys_base.js
No further changes required.
phpcs.xml (1)
16-16: Appropriate removal of JS and CSS from PHPCS scope.Removing JavaScript and CSS extensions from PHPCS scanning is correct, as these file types are now handled by dedicated linting tools (ESLint and Stylelint) introduced in this PR. This improves the separation of concerns between PHP and frontend asset linting.
.stylelintrc.js (1)
1-39: LGTM! Well-structured Stylelint configuration.The configuration appropriately extends the standard ruleset, enforces property alphabetical ordering, and allows necessary Sass at-rules while disabling overly strict pattern rules. This provides a good balance between code quality enforcement and developer flexibility.
web/modules/custom/ys_base/src/Plugin/Block/CounterBlock.php (2)
25-35: LGTM! Proper block implementation with correct render array structure.The build method correctly returns a render array with the theme hook, initial counter value, and library attachment. This follows Drupal best practices for block plugins.
37-43: LGTM! Appropriate cache disabling for interactive content.Disabling caching by returning 0 is correct for this interactive counter block, ensuring the block updates dynamically without cache interference.
tests/behat/features/counter.feature (1)
8-40: LGTM! Comprehensive test coverage for counter functionality.The test scenarios thoroughly cover counter block visibility, button interactions, negative values, reset functionality, and persistence across page reloads. The test steps are well-structured and use appropriate selectors.
.circleci/config.yml (3)
266-266: LGTM! Added yarn install for frontend dependencies.Adding
yarn install --frozen-lockfileafter Composer installation ensures frontend dependencies are available for the new linting steps. Using--frozen-lockfileis appropriate for CI environments.
269-269: LGTM! Fixed typo in step name.The correction from "Validate Composer configuration normalized" to "Validate Composer configuration is normalized" improves clarity.
297-304: LGTM! Proper separation of module and theme linting.Splitting NodeJS linting into separate steps for module code and theme code allows for different linting configurations while maintaining the existing conditional failure handling. This aligns with the new frontend asset management setup.
web/modules/custom/ys_base/js/ys_base.js (1)
9-74: LGTM! Well-implemented counter behavior with proper persistence.The Drupal behavior correctly implements counter functionality with localStorage persistence, proper event handling, visual feedback, and prevents multiple initialization. The logic for increment, decrement, and reset operations is sound.
.ahoy.yml (1)
204-209: Guard theme-specific lint when no custom theme present
IfDRUPAL_THEMEis empty theyarn run --cwd=…call errors. Wrap it with[ -n "${DRUPAL_THEME}" ] && …(unless the Vortex pre-processor already strips it).
| *.min.js | ||
| *.min.css No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use recursive pattern so minified assets nested in sub-dirs are ignored too
*.min.js and *.min.css only match files in the repo root.
Switch to a recursive glob so anything in deeper paths (web/**, themes/**, etc.) is excluded as well.
-*.min.js
-*.min.css
+**/*.min.js
+**/*.min.css📝 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.
| *.min.js | |
| *.min.css | |
| **/*.min.js | |
| **/*.min.css |
🤖 Prompt for AI Agents
In the .eslintignore file at lines 9 to 10, the patterns '*.min.js' and
'*.min.css' only ignore minified files in the root directory. To also ignore
minified files in all subdirectories, update these patterns to use a recursive
glob like '**/*.min.js' and '**/*.min.css'. This change ensures minified assets
nested in any folder are excluded from linting.
| docker compose exec $(env | cut -f1 -d= | sed 's/^/-e /') -T cli bash -c "yarn install --frozen-lockfile" | ||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Missing dependency caching for Yarn install
yarn install --frozen-lockfile will download the full npm registry every run.
Add a actions/cache step (similar to Composer) keyed on yarn.lock to cut CI time drastically and avoid rate-limiting.
🤖 Prompt for AI Agents
In .github/workflows/build-test-deploy.yml around lines 254 to 255, the workflow
runs `yarn install --frozen-lockfile` without caching dependencies, causing
repeated full downloads and slower CI. Add an `actions/cache` step before this
command that caches the Yarn cache directory, using `yarn.lock` as the cache
key, to speed up installs and reduce network usage.
| - name: Lint module code with NodeJS linters | ||
| run: docker compose exec -T cli bash -c "yarn run lint" | ||
| continue-on-error: ${{ vars.VORTEX_CI_NODEJS_LINT_IGNORE_FAILURE == '1' }} | ||
|
|
||
| #;< DRUPAL_THEME | ||
| - name: Lint code with NodeJS linters | ||
| - name: Lint theme code with NodeJS linters | ||
| run: docker compose exec -T cli bash -c "yarn --cwd=\${WEBROOT}/themes/custom/\${DRUPAL_THEME} run lint" | ||
| continue-on-error: ${{ vars.VORTEX_CI_NODEJS_LINT_IGNORE_FAILURE == '1' }} | ||
| #;> DRUPAL_THEME |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Theme lint step runs without installing its dependencies
You run yarn run lint for the root package.json (good) but call the theme linter straight away:
yarn --cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} run lintUnless the theme has already executed yarn install, this will fail or perform an ad-hoc, non-cached install.
Insert a dedicated install step first (mirrors the module install):
+ - name: Install NodeJS deps for theme
+ run: docker compose exec -T cli bash -c "yarn --cwd=\${WEBROOT}/themes/custom/\${DRUPAL_THEME} install --frozen-lockfile"
+
- name: Lint theme code with NodeJS linters🤖 Prompt for AI Agents
In .github/workflows/build-test-deploy.yml around lines 284 to 292, the theme
lint step runs the linter without first installing the theme's dependencies,
which can cause failures or inefficient installs. Add a step before the theme
lint step to run `yarn install` in the theme directory using `yarn
--cwd=${WEBROOT}/themes/custom/${DRUPAL_THEME} install` to ensure dependencies
are installed before linting.
| "scripts": { | ||
| "lint-js": "eslint web/modules/custom --ext .js --max-warnings=0", | ||
| "lint-css": "stylelint \"web/modules/custom/**/*.css\"", | ||
| "lint": "npm run lint-js && npm run lint-css", | ||
| "lint-fix-js": "eslint web/modules/custom --ext .js --fix", | ||
| "lint-fix-css": "stylelint \"web/modules/custom/**/*.css\" --fix", | ||
| "lint-fix": "npm run lint-fix-js && npm run lint-fix-css" | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid spawning npm inside Yarn scripts
yarn run lint ends up calling npm run …, losing Yarn’s lock-file guarantees and adding ~5 s overhead per script.
- "lint": "npm run lint-js && npm run lint-css",
- "lint-fix": "npm run lint-fix-js && npm run lint-fix-css"
+ "lint": "yarn lint-js && yarn lint-css",
+ "lint-fix": "yarn lint-fix-js && yarn lint-fix-css"Also consider replacing npm in the CI commands with yarn to stay consistent.
📝 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.
| "scripts": { | |
| "lint-js": "eslint web/modules/custom --ext .js --max-warnings=0", | |
| "lint-css": "stylelint \"web/modules/custom/**/*.css\"", | |
| "lint": "npm run lint-js && npm run lint-css", | |
| "lint-fix-js": "eslint web/modules/custom --ext .js --fix", | |
| "lint-fix-css": "stylelint \"web/modules/custom/**/*.css\" --fix", | |
| "lint-fix": "npm run lint-fix-js && npm run lint-fix-css" | |
| }, | |
| "scripts": { | |
| "lint-js": "eslint web/modules/custom --ext .js --max-warnings=0", | |
| "lint-css": "stylelint \"web/modules/custom/**/*.css\"", | |
| - "lint": "npm run lint-js && npm run lint-css", | |
| + "lint": "yarn lint-js && yarn lint-css", | |
| "lint-fix-js": "eslint web/modules/custom --ext .js --fix", | |
| "lint-fix-css": "stylelint \"web/modules/custom/**/*.css\" --fix", | |
| - "lint-fix": "npm run lint-fix-js && npm run lint-fix-css" | |
| + "lint-fix": "yarn lint-fix-js && yarn lint-fix-css" | |
| }, |
🤖 Prompt for AI Agents
In package.json lines 10 to 17, the lint scripts use "npm run" commands inside
Yarn scripts, which causes loss of Yarn's lock-file guarantees and adds
overhead. Replace all instances of "npm run" with "yarn" to ensure consistency
and better performance. Also update any related CI commands to use "yarn"
instead of "npm" for consistency across the project.
closes #1754
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation