Added support for returning vite assets in their correct group#1481
Added support for returning vite assets in their correct group#1481
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaces the assets function argument in modules/cms/twig/StylesNode.php from 'vite' to 'css'. Enhances modules/system/traits/AssetMaker.php to detect asset types (.css/.js), filter Vite bundle entrypoints by type, and emit Vite::tags for matching CSS and JS entrypoints; adds protected getAssetType(string $asset). Adds Vite-focused tests and helpers: modules/cms/tests/classes/TwigExtensionTest.php and modules/system/tests/traits/AssetMakerTest.php include fixture setup/teardown and multiple new unit tests validating CSS/JS and Vite integration. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/system/traits/AssetMaker.php (1)
76-86: ⚡ Quick winDeduplicate the two Vite emission blocks.
The CSS and JS branches contain the same loop modulo the type literal. Extracting a small helper keeps
makeAssets()readable and makes future tweaks (ordering, additional types, error handling) a one-place change. It also pairs naturally with the future-extensible intent called out forgetAssetType()in the PR description.♻️ Proposed extraction
if ($type == null || $type == 'css') { foreach ($this->orderAssets($this->assets['css']) as $asset) { ... $result .= '<link' . $attributes . '>' . PHP_EOL; } - - foreach ($this->assets['vite'] as $asset) { - $asset['attributes']['entrypoints'] = array_filter( - $asset['attributes']['entrypoints'], - fn ($entrypoint) => $this->getAssetType($entrypoint) === 'css' - ); - - if ($asset['attributes']['entrypoints']) { - $result .= Vite::tags($asset['attributes']['entrypoints'], $asset['path']); - } - } + $result .= $this->makeViteAssetsForType('css'); } ... if ($type == null || $type == 'js') { foreach ($this->orderAssets($this->assets['js']) as $asset) { ... $result .= '<script' . $attributes . '></script>' . PHP_EOL; } - - foreach ($this->assets['vite'] as $asset) { - $asset['attributes']['entrypoints'] = array_filter( - $asset['attributes']['entrypoints'], - fn ($entrypoint) => $this->getAssetType($entrypoint) === 'js' - ); - - if ($asset['attributes']['entrypoints']) { - $result .= Vite::tags($asset['attributes']['entrypoints'], $asset['path']); - } - } + $result .= $this->makeViteAssetsForType('js'); }protected function makeViteAssetsForType(string $type): string { $result = ''; foreach ($this->assets['vite'] as $asset) { $entrypoints = array_filter( $asset['attributes']['entrypoints'] ?? [], fn ($entrypoint) => $this->getAssetType($entrypoint) === $type ); if ($entrypoints) { $result .= Vite::tags($entrypoints, $asset['path']); } } return $result; }Also applies to: 116-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/system/traits/AssetMaker.php` around lines 76 - 86, The two identical loops over $this->assets['vite'] that filter entrypoints by type should be extracted into a helper (e.g. protected function makeViteAssetsForType(string $type): string) and the two inlined blocks in makeAssets() replaced with calls to that helper for 'css' and 'js'; the helper should pull entrypoints via $asset['attributes']['entrypoints'] ?? [], use array_filter with $this->getAssetType($entrypoint) === $type, and call Vite::tags($entrypoints, $asset['path']) when non-empty, returning the concatenated string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/system/traits/AssetMaker.php`:
- Around line 484-495: getAssetType currently returns only 'js' for .js and
'css' for .css, causing .ts/.tsx/.jsx/.mjs/.cjs/.vue and preprocessor styles
.scss/.sass to be dropped; update getAssetType(string $asset): ?string to also
treat .ts, .tsx, .jsx, .mjs, .cjs, .vue as 'js' and .scss, .sass (and keep .css)
as 'css' so makeAssets('js') and makeAssets('css') include typical Vite
entrypoints; while editing, keep the function name getAssetType and its return
semantics so existing callers (makeAssets, the Vite emission logic) continue to
work—optionally factor repeated Vite-emission filtering into a shared helper if
you want to remove duplication later.
---
Nitpick comments:
In `@modules/system/traits/AssetMaker.php`:
- Around line 76-86: The two identical loops over $this->assets['vite'] that
filter entrypoints by type should be extracted into a helper (e.g. protected
function makeViteAssetsForType(string $type): string) and the two inlined blocks
in makeAssets() replaced with calls to that helper for 'css' and 'js'; the
helper should pull entrypoints via $asset['attributes']['entrypoints'] ?? [],
use array_filter with $this->getAssetType($entrypoint) === $type, and call
Vite::tags($entrypoints, $asset['path']) when non-empty, returning the
concatenated string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e53cd79b-1aef-488d-accb-4c14090e1d2b
📒 Files selected for processing (2)
modules/cms/twig/StylesNode.phpmodules/system/traits/AssetMaker.php
💤 Files with no reviewable changes (1)
- modules/cms/twig/StylesNode.php
There was a problem hiding this comment.
🧹 Nitpick comments (3)
modules/system/tests/traits/AssetMakerTest.php (2)
53-53: 💤 Low valueNaming nit:
setUpViteFixturemimics a PHPUnit lifecycle hook.
setUp*-prefixed methods are typically auto-invoked PHPUnit hooks. Since this helper is called manually from each test, a name likeprepareViteFixture()orbootViteFixture()would reduce reader surprise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/system/tests/traits/AssetMakerTest.php` at line 53, The helper method setUpViteFixture() should be renamed to avoid resembling PHPUnit lifecycle hooks; rename the method (e.g., prepareViteFixture or bootViteFixture) and update every test that calls setUpViteFixture to the new name, including any references in AssetMakerTest and related fixtures or trait usages so the helper remains manually-invoked and reader expectations are clear.
24-80: ⚡ Quick winConsider extracting Vite fixture scaffolding into a shared trait.
These three constants, the entire
tearDown()cleanup block, and the package-registration / hot-file portion ofsetUpViteFixture()are duplicated near-verbatim inmodules/cms/tests/classes/TwigExtensionTest.php. Centralizing them into a shared test trait (e.g.,System\Tests\Concerns\WithViteFixture) would keep cleanup in lockstep across both test files and avoid drift if the fixture path or hot URL ever changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/system/tests/traits/AssetMakerTest.php` around lines 24 - 80, Duplicate Vite fixture scaffolding (constants VITE_FIXTURE_PACKAGE, VITE_FIXTURE_THEME_PATH, VITE_HOT_URL, the tearDown() cleanup and the package-registration/hot-file logic in setUpViteFixture()) exists in AssetMakerTest and TwigExtensionTest; extract these into a shared test trait (e.g., System\Tests\Concerns\WithViteFixture) that defines the three constants, provides a protected tearDownViteFixture() or tearDown() implementation and a setUpViteFixture() method containing the PackageManager registration and hot-file creation, then have AssetMakerTest and TwigExtensionTest use that trait and remove the duplicated constants and methods from those classes so both tests share the single canonical implementation.modules/cms/tests/classes/TwigExtensionTest.php (1)
15-31: ⚡ Quick winDuplicated Vite fixture constants and tearDown — same root cause as in
AssetMakerTest.These constants and the
tearDown()body match the ones inmodules/system/tests/traits/AssetMakerTest.php. A shared trait would let both suites converge on a single cleanup implementation. (See the corresponding comment onAssetMakerTest.php.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/cms/tests/classes/TwigExtensionTest.php` around lines 15 - 31, Extract the duplicated constants and cleanup logic into a shared trait (e.g., AssetFixtureCleanupTrait) that defines the constants VITE_FIXTURE_PACKAGE, VITE_FIXTURE_THEME_PATH, VITE_HOT_URL and a protected method cleanupViteFixtures() which contains the file/directory removal logic; then update TwigExtensionTest::tearDown() and AssetMakerTest::tearDown() to call $this->cleanupViteFixtures() before calling parent::tearDown(); ensure the trait method is protected and referenced by the class tests so teardown behavior is identical and duplication is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@modules/cms/tests/classes/TwigExtensionTest.php`:
- Around line 15-31: Extract the duplicated constants and cleanup logic into a
shared trait (e.g., AssetFixtureCleanupTrait) that defines the constants
VITE_FIXTURE_PACKAGE, VITE_FIXTURE_THEME_PATH, VITE_HOT_URL and a protected
method cleanupViteFixtures() which contains the file/directory removal logic;
then update TwigExtensionTest::tearDown() and AssetMakerTest::tearDown() to call
$this->cleanupViteFixtures() before calling parent::tearDown(); ensure the trait
method is protected and referenced by the class tests so teardown behavior is
identical and duplication is removed.
In `@modules/system/tests/traits/AssetMakerTest.php`:
- Line 53: The helper method setUpViteFixture() should be renamed to avoid
resembling PHPUnit lifecycle hooks; rename the method (e.g., prepareViteFixture
or bootViteFixture) and update every test that calls setUpViteFixture to the new
name, including any references in AssetMakerTest and related fixtures or trait
usages so the helper remains manually-invoked and reader expectations are clear.
- Around line 24-80: Duplicate Vite fixture scaffolding (constants
VITE_FIXTURE_PACKAGE, VITE_FIXTURE_THEME_PATH, VITE_HOT_URL, the tearDown()
cleanup and the package-registration/hot-file logic in setUpViteFixture())
exists in AssetMakerTest and TwigExtensionTest; extract these into a shared test
trait (e.g., System\Tests\Concerns\WithViteFixture) that defines the three
constants, provides a protected tearDownViteFixture() or tearDown()
implementation and a setUpViteFixture() method containing the PackageManager
registration and hot-file creation, then have AssetMakerTest and
TwigExtensionTest use that trait and remove the duplicated constants and methods
from those classes so both tests share the single canonical implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07d008b1-0323-427d-b35f-43563b53c25e
📒 Files selected for processing (2)
modules/cms/tests/classes/TwigExtensionTest.phpmodules/system/tests/traits/AssetMakerTest.php
|
It works as expected for me. 👍 |
3 participants
This PR adds support for returning vite assets in their correct groups.
This is done so that CSS/JS added via
->addVite()is correctly rendered via the{% styles %}&{% scripts %}tags.The
getAssetType()method is a little fluffy as it will probably be extended later.Summary by CodeRabbit
Refactor
Tests