Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Node.js-based frontend build pipeline (Webpack Encore, Vue, Tailwind/PostCSS), integrates its build into Docker and docker-compose startup, updates Composer to include a CORS bundle and a Composer script to build frontend assets, adds test bootstrap and a test assertion, and updates ignore files. ChangesWeb frontend build & runtime wiring
Testing bootstrap & assertions
sequenceDiagram
participant Dev as Developer
participant Repo as Git repo
participant Docker as Docker build
participant Node as Node/Yarn
participant Composer as Composer
participant Apache as Apache container
Dev->>Repo: commit package.json, webpack.config.js, composer.json, Dockerfile, docker-compose.yml
Docker->>Repo: copy `composer.json`, `composer.lock`, `package.json`, `yarn.lock`
Docker->>Node: install Node.js 20 and global yarn
Node->>Docker: run `yarn install` (during build or runtime script)
Composer->>Node: `composer run-script build-web-frontend-assets` -> invokes `yarn build:web-frontend`
Node->>Docker: produce `public/build` assets
Apache->>Docker: container start command runs asset build, prepares dirs, then `apache2-foreground`
Dev->>Repo: run tests (PHPUnit) which use `tests/bootstrap.php` for initialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.gitignore (1)
1-15:⚠️ Potential issue | 🟠 MajorRemove the catch-all ignore unless force-adding every new file is intentional.
With
*on Line 1 and no negate rules, Git ignores every new untracked file in the repository. That means future source/config additions requiregit add -f, and the new/node_modules/entry is redundant anyway.Suggested change
-* *.bak /.idea/ /.project /.webprj🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 1 - 15, Remove the catch-all ignore pattern (*) from .gitignore (or replace it with explicit patterns) so new untracked files are not globally ignored; specifically remove or change the '*' entry and rely on targeted patterns like '*~' for editor backups instead. Also remove the redundant '/node_modules/' entry if your repo already manages node_modules elsewhere, then verify the remaining patterns (e.g., '*~', '*.bak', '/var/') only match intended files and commit the updated .gitignore.
🧹 Nitpick comments (1)
postcss.config.js (1)
2-4:autoprefixerlooks redundant in this Tailwind 4 pipeline.With Tailwind v4's
@tailwindcss/postcss, vendor prefixing is already handled by the built-in Lightning CSS step. Keepingautoprefixerhere adds an extra pass for no clear benefit, so I'd simplify this plugin list and drop the matching dependency as well.Suggested change
module.exports = { plugins: [ require('@tailwindcss/postcss'), - require('autoprefixer'), ], };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@postcss.config.js` around lines 2 - 4, Remove the redundant autoprefixer entry from the plugins array in postcss.config.js: delete the require('autoprefixer') item and leave only require('@tailwindcss/postcss') in the plugins list, and also remove the autoprefixer dependency from package.json to avoid an unused package; after the change, run the build to verify no regressions in the pipeline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Around line 47-51: The composer file currently pins several packages to
unstable branch names ("phplist/core" -> dev-dev, "phplist/web-frontend" ->
dev-feat/campaigns, "tatevikgr/rest-api" -> dev-dev) which are moving targets;
replace these dev-branch references with concrete, versioned tags or commit
hashes (e.g., semantic versions or specific shas) for "phplist/core",
"phplist/rest-api", "phplist/web-frontend", and "tatevikgr/rest-api" before
merging. Also update the "build-web-frontend-assets" script to run yarn with a
lockfile present (ensure a yarn.lock is committed and run "yarn install
--frozen-lockfile" or equivalent) to prevent drift, and add "nelmio/cors-bundle"
as a Composer dependency to match the registered bundle
Nelmio\CorsBundle\NelmioCorsBundle in extra.phplist/core.bundles so runtime
resolution is explicit.
- Around line 111-115: composer.json currently references
Nelmio\CorsBundle\NelmioCorsBundle in the bundles list but does not declare
"nelmio/cors-bundle" as a direct dependency, relying on phplist/rest-api to pull
it transitively; add "nelmio/cors-bundle" to the "require" section of
composer.json with an appropriate stable constraint, run composer update/install
to lock it, and ensure the bundle entry (Nelmio\CorsBundle\NelmioCorsBundle)
remains in your kernel/bundles registration so the package and configuration
stay consistent even if phplist/rest-api changes.
In `@docker-compose.yml`:
- Around line 22-25: The docker-compose volumes bind-mount .:/var/www/html which
overwrites image-built files (vendor/, public/build, ownership fixes); remove
the .:/var/www/html mount and only bind the specific runtime dirs (keep
./var/logs:/var/www/html/var/log and ./var/cache:/var/www/html/var/cache or add
other specific app state dirs) so that the Composer-installed vendor tree and
generated assets from the image are preserved and Dockerfile ownership steps
still apply; update the volumes section by deleting the .:/var/www/html entry
and keeping or adding explicit mounts for only the mutable runtime directories
referenced in the diff (./var/logs and ./var/cache).
In `@README.md`:
- Around line 154-160: The README is missing a prerequisite note that `composer
run-script build-web-frontend-assets` invokes the `yarn` command; update the
README section around the phplist/web-frontend → public/build instructions to
add a short prerequisite sentence stating that Node.js and Yarn must be
installed locally (or point to Docker usage) before running `composer run-script
build-web-frontend-assets`, mentioning `yarn` explicitly so users know why those
tools are required.
---
Outside diff comments:
In @.gitignore:
- Around line 1-15: Remove the catch-all ignore pattern (*) from .gitignore (or
replace it with explicit patterns) so new untracked files are not globally
ignored; specifically remove or change the '*' entry and rely on targeted
patterns like '*~' for editor backups instead. Also remove the redundant
'/node_modules/' entry if your repo already manages node_modules elsewhere, then
verify the remaining patterns (e.g., '*~', '*.bak', '/var/') only match intended
files and commit the updated .gitignore.
---
Nitpick comments:
In `@postcss.config.js`:
- Around line 2-4: Remove the redundant autoprefixer entry from the plugins
array in postcss.config.js: delete the require('autoprefixer') item and leave
only require('@tailwindcss/postcss') in the plugins list, and also remove the
autoprefixer dependency from package.json to avoid an unused package; after the
change, run the build to verify no regressions in the pipeline.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0f46aa4-92ea-47c6-9646-c91ad6820f82
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.gitignoreDockerfileREADME.mdcomposer.jsondocker-compose.ymlpackage.jsonpostcss.config.jswebpack.config.js
Comment on lines
+154
to
+160
| To compile and publish the `phplist/web-frontend` assets into | ||
| `public/build` (used by the web frontend Twig templates), | ||
| run: | ||
|
|
||
| ```bash | ||
| composer run-script build-web-frontend-assets | ||
| ``` |
There was a problem hiding this comment.
Document the local Node/Yarn prerequisite for this command.
composer run-script build-web-frontend-assets shells out to yarn, so on non-Docker setups this fails unless Node.js and Yarn are installed first. A short prerequisite note here would prevent a confusing first-run error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 154 - 160, The README is missing a prerequisite note
that `composer run-script build-web-frontend-assets` invokes the `yarn` command;
update the README section around the phplist/web-frontend → public/build
instructions to add a short prerequisite sentence stating that Node.js and Yarn
must be installed locally (or point to Docker usage) before running `composer
run-script build-web-frontend-assets`, mentioning `yarn` explicitly so users
know why those tools are required.
TatevikGr
force-pushed
the
dev
branch
2 times, most recently
from
4ce3d92 to
3c1887e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docker-compose.yml (1)
16-17: Parameterize base URLs instead of hard-codinghttp://app/.Using a fixed internal hostname can break externally generated links in non-local deployments. Prefer overrideable env-default expansion.
♻️ Proposed change
- REST_API_BASE_URL: 'http://app/' - FRONT_END_BASE_URL: 'http://app/' + REST_API_BASE_URL: ${REST_API_BASE_URL:-http://app/} + FRONT_END_BASE_URL: ${FRONT_END_BASE_URL:-http://app/}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 16 - 17, The docker-compose service currently hard-codes REST_API_BASE_URL and FRONT_END_BASE_URL to "http://app/", which breaks externally generated links; change these to use environment-variable expansion with sensible defaults so deployers can override them (e.g., use ${REST_API_BASE_URL:-http://app/} and ${FRONT_END_BASE_URL:-http://app/} in the docker-compose env entries). Update the REST_API_BASE_URL and FRONT_END_BASE_URL entries to reference those env vars so external deployments can supply proper public-facing base URLs without editing the compose file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Composer/SecurityConfigScriptHandler.php`:
- Around line 43-49: The patch in SecurityConfigScriptHandler.php currently sets
a catch-all dev firewall ($configuration['security']['firewalls']['dev']) to
pattern '^/' and security=false, which can disable auth in production; change
the handler to restrict the pattern to dev-only routes (e.g., '^/_') and guard
applying this default with an environment check (only apply when APP_ENV ===
'dev' or when a composer extra/dev flag is present) and/or skip modifying the
firewall if a non-dev environment is detected; update any associated README
notes to document the ordering risk and that the script only applies dev-pattern
defaults in development.
- Around line 24-27: The code in SecurityConfigScriptHandler should explicitly
handle file I/O and YAML parsing errors: after calling
file_get_contents($configurationFilePath) check for a false return and throw or
log a clear exception; wrap Yaml::parse(...) in a try-catch that catches
Symfony\Component\Yaml\Exception\ParseException and rethrows or reports a
descriptive error including the file path and parse message; avoid relying
solely on is_readable() (TOCTOU) by treating read failures as fatal when
file_get_contents fails; and when writing with file_put_contents(...) check for
a === false return and handle that failure (throw/log) so write errors are
detected. Reference these symbols: $configurationFilePath, file_get_contents,
Yaml::parse, ParseException, and file_put_contents in
SecurityConfigScriptHandler.php.
In `@tests/Integration/Composer/ScriptsTest.php`:
- Around line 128-133: The test
testModulesConfigurationFileContainsSecurityConfiguration is too weak because it
only checks for the 'security:' substring; instead parse the
config/config_modules.yml into a PHP array (e.g. using
Symfony\Component\Yaml\Yaml::parseFile or yaml_parse) and assert that a
top-level 'security' key exists and is an array, then assert the expected nested
keys and values produced by SecurityConfigScriptHandler (for example presence of
'firewalls', 'providers', and any required firewall names or settings) to ensure
the YAML structure and specific entries match what the handler generates.
---
Nitpick comments:
In `@docker-compose.yml`:
- Around line 16-17: The docker-compose service currently hard-codes
REST_API_BASE_URL and FRONT_END_BASE_URL to "http://app/", which breaks
externally generated links; change these to use environment-variable expansion
with sensible defaults so deployers can override them (e.g., use
${REST_API_BASE_URL:-http://app/} and ${FRONT_END_BASE_URL:-http://app/} in the
docker-compose env entries). Update the REST_API_BASE_URL and FRONT_END_BASE_URL
entries to reference those env vars so external deployments can supply proper
public-facing base URLs without editing the compose file.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0827e2a0-a9c0-4b5f-8830-422b615c6086
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
composer.jsondocker-compose.ymlphpunit.xml.distsrc/Composer/SecurityConfigScriptHandler.phptests/Integration/Composer/ScriptsTest.phptests/bootstrap.php
✅ Files skipped from review due to trivial changes (1)
- phpunit.xml.dist
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/bootstrap.php
- composer.json
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
composer.json (1)
94-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
yarn installwithout--frozen-lockfilecauses version drift.This is still unaddressed from the previous review cycle. Without
--frozen-lockfile(or--immutablefor yarn berry),yarn installwill silently upgrade packages whenever the registry has newer compatible versions, making the frontend build non-reproducible. A committedyarn.lockis only meaningful if the install is forced to respect it.🔒 Proposed fix
"build-web-frontend-assets": [ - "yarn install", + "yarn install --frozen-lockfile", "yarn build:web-frontend" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composer.json` around lines 94 - 97, The "build-web-frontend-assets" npm script currently runs "yarn install" which allows dependency resolution drift; update the script entry named build-web-frontend-assets so it runs a lockfile-respecting install (e.g., replace "yarn install" with "yarn install --frozen-lockfile" for Classic Yarn or "yarn install --immutable" for Yarn Berry) to ensure the committed yarn.lock is honored during frontend builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 30-37: The runtime command in docker-compose currently runs
composer run-script build-web-frontend-assets (which performs yarn install +
webpack production) before exec apache2-foreground, causing long cold starts and
network dependency; move the frontend build into the image build instead by
running the same build step after the image's composer install during Dockerfile
build (so Node/yarn are used at build time and public/build is baked into the
image) and then remove composer run-script build-web-frontend-assets from the
docker-compose command so the container just prepares caches/logs
(mkdir/chown/chmod) and then exec apache2-foreground; update references to the
build script name (composer run-script build-web-frontend-assets) and ensure
public/build is produced at Dockerfile time.
In `@Dockerfile`:
- Around line 22-24: The RUN layer that installs Node (`RUN curl -fsSL
https://deb.nodesource.com/setup_20.x | bash - \ && apt install -y nodejs \ &&
npm install -g yarn`) should use apt-get (not apt) and remove APT lists in the
same layer to avoid larger images: change `apt install -y nodejs` to `apt-get
update && apt-get install -y --no-install-recommends nodejs` (keep the curl
setup step), then chain `&& rm -rf /var/lib/apt/lists/*` at the end of that RUN;
ensure the entire sequence (curl | bash, apt-get update, apt-get install, npm
install -g yarn, and the rm -rf cleanup) remains in one RUN instruction.
---
Duplicate comments:
In `@composer.json`:
- Around line 94-97: The "build-web-frontend-assets" npm script currently runs
"yarn install" which allows dependency resolution drift; update the script entry
named build-web-frontend-assets so it runs a lockfile-respecting install (e.g.,
replace "yarn install" with "yarn install --frozen-lockfile" for Classic Yarn or
"yarn install --immutable" for Yarn Berry) to ensure the committed yarn.lock is
honored during frontend builds.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2cd6f035-4129-432f-b8b1-87a7ac8c734e
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.dockerignoreDockerfilecomposer.jsondocker-compose.ymlpackage.json
💤 Files with no reviewable changes (1)
- .dockerignore
Comment on lines
+22
to
+24
| RUN curl -fsSL https://deb.nodesource.com/setup_20.x | bash - \ | ||
| && apt install -y nodejs \ | ||
| && npm install -g yarn |
There was a problem hiding this comment.
Use apt-get instead of apt, and clean up apt lists in the same layer.
Two issues in this RUN block:
- Line 23 uses
apt install—aptis designed for interactive terminal use and is explicitly documented as having an unstable CLI interface not suitable for scripts. Useapt-getconsistently with the rest of the Dockerfile. - No
rm -rf /var/lib/apt/lists/*at the end of this layer, inflating the image size with package index data.
🔧 Proposed fix
RUN curl -fsSL https://deb.nodesource.com/setup_20.x | bash - \
- && apt install -y nodejs \
- && npm install -g yarn
+ && apt-get install -y --no-install-recommends nodejs \
+ && npm install -g yarn \
+ && rm -rf /var/lib/apt/lists/*📝 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.
Suggested change
| RUN curl -fsSL https://deb.nodesource.com/setup_20.x | bash - \ | |
| && apt install -y nodejs \ | |
| && npm install -g yarn | |
| RUN curl -fsSL https://deb.nodesource.com/setup_20.x | bash - \ | |
| && apt-get install -y --no-install-recommends nodejs \ | |
| && npm install -g yarn \ | |
| && rm -rf /var/lib/apt/lists/* |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 22 - 24, The RUN layer that installs Node (`RUN curl
-fsSL https://deb.nodesource.com/setup_20.x | bash - \ && apt install -y nodejs
\ && npm install -g yarn`) should use apt-get (not apt) and remove APT lists in
the same layer to avoid larger images: change `apt install -y nodejs` to
`apt-get update && apt-get install -y --no-install-recommends nodejs` (keep the
curl setup step), then chain `&& rm -rf /var/lib/apt/lists/*` at the end of that
RUN; ensure the entire sequence (curl | bash, apt-get update, apt-get install,
npm install -g yarn, and the rm -rf cleanup) remains in one RUN instruction.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Thanks for contributing to phpList!
Summary by CodeRabbit
New Features
Chores
Tests
Documentation