Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR introduces Node.js frontend build infrastructure (Webpack, Vue, Tailwind CSS, PostCSS) to the application, updates Docker setup with Node.js and yarn installation, modifies phplist package dependencies to development branches, adds CORS bundle, configures new web asset build scripts, and updates test bootstrap configuration with output buffering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
+47
to
+51
| "phplist/core": "dev-dev", | ||
| "phplist/rest-api": "dev-dev", | ||
| "phplist/web-frontend": "dev-feat/campaigns", | ||
| "doctrine/orm": "^3.3", | ||
| "tatevikgr/rest-api-client": "dev-ISSUE-357", | ||
| "tatevikgr/rest-api-client": "dev-dev", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n composer.json | head -60Repository: phpList/base-distribution
Length of output: 2306
🏁 Script executed:
cat -n composer.json | sed -n '90,120p'Repository: phpList/base-distribution
Length of output: 1168
🏁 Script executed:
# Check for all dev branch references in composer.json
rg 'dev-[a-z]' composer.json -A 0 -B 0Repository: phpList/base-distribution
Length of output: 345
🏁 Script executed:
# Check if nelmio/cors-bundle is declared as a dependency
rg 'nelmio/cors-bundle' composer.jsonRepository: phpList/base-distribution
Length of output: 51
Pin these package inputs before merging this branch into main.
Multiple dev branch references (dev-dev, dev-feat/campaigns, dev-main, dev-master) on lines 47–52 are moving targets, so the same composer install can resolve different code later without any change in this repo. For a base distribution, that makes releases and bug reports hard to reproduce.
Additionally:
- The
build-web-frontend-assetsscript (lines 93–96) runsyarn installwithout a lockfile, causing frontend asset builds to drift. Nelmio\CorsBundle\NelmioCorsBundleis registered inextra.phplist/core.bundles(line 113) butnelmio/cors-bundleis not declared as a Composer dependency, creating a fragile runtime dependency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composer.json` around lines 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.
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
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
Release Notes
New Features
Chores
Documentation