Conversation
📝 WalkthroughWalkthroughThis PR integrates a Vue 3 frontend build system (Webpack Encore, Tailwind, CKEditor), adds npm/package tooling, wires a Composer script to build frontend assets, updates the Dockerfile to install Node/Yarn and build assets during image creation, adjusts docker-compose and Apache to run on port 8081 with mounted var volumes, adds a CI publish workflow, and updates test bootstrap/phpunit configuration. ChangesWeb Frontend Build Pipeline Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.gitignore (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove repository-wide wildcard ignore at root.
Line 1 (
*) causes Git to ignore essentially all new files, which can break normal staging workflows and makes subsequent rules ineffective unless explicit negations are added.Suggested fix
-* *.bak /.idea/ /.project /.webprj /bin/ /config/bundles.yml /config/config_modules.yml /config/parameters.yml /config/routing_modules.yml /nbproject /public/ /var/ /vendor/ /node_modules/ .DS_Store .vagrant .phpunit.result.cache🤖 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 @.gitignore at line 1, Remove the repository-wide wildcard entry '*' from .gitignore (the root wildcard that ignores all new files) and replace it with either specific ignore patterns (e.g., editor backups, build artifacts, OS files) or move broad personal ignores to a global git exclude via git config --global core.excludesfile; if you intended to ignore everything except a few files, convert to explicit negation rules instead of '*'. Ensure you update the existing .gitignore entry that currently contains '*' to one of these safer options so normal staging and subsequent ignore rules work correctly.Dockerfile (1)
47-50:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
composer installruns scripts beforebin/consoleexists.
composer installis executed beforeCOPY . ., but post-install scripts callphp bin/console. This can break the image build.🐛 Proposed fix
-RUN composer install --no-dev --no-interaction --prefer-dist --optimize-autoloader +RUN composer install --no-dev --no-interaction --prefer-dist --optimize-autoloader --no-scripts # Copy the rest of the application (except files ignored by .dockerignore) COPY . . + +# Run project scripts after app sources (including bin/console) are present +RUN composer run-script post-install-cmd🤖 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 `@Dockerfile` around lines 47 - 50, The Dockerfile runs "composer install" before "COPY . .", which triggers Composer post-install scripts that call "php bin/console" when bin/console doesn't exist yet; either move the "COPY . ." step so that the application files (including bin/console) are copied before running "composer install", or change the composer step to "composer install --no-scripts --no-dev --no-interaction --prefer-dist --optimize-autoloader" and then run "composer run scripts" (or "composer install" without --no-scripts) after the COPY so post-install scripts can find bin/console.
🧹 Nitpick comments (1)
postcss.config.js (1)
2-5: ⚡ Quick winRemove redundant
autoprefixerfrom the Tailwind v4 pipeline.With Tailwind v4,
@tailwindcss/postcssautomatically handles vendor prefixing and minification through Lightning CSS, making Autoprefixer unnecessary.♻️ Proposed change
module.exports = { plugins: [ require('`@tailwindcss/postcss`'), - require('autoprefixer'), ], };// package.json (devDependencies) - "autoprefixer": "^10.4.27",🤖 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 `@postcss.config.js` around lines 2 - 5, The plugins array in postcss.config.js includes require('autoprefixer') which is redundant with Tailwind v4's require('`@tailwindcss/postcss`'); remove the require('autoprefixer') entry from the plugins array so only require('`@tailwindcss/postcss`') remains, and ensure the plugins array syntax remains valid after removal.
🤖 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 `@composer.json`:
- Around line 94-97: The build script "build-web-frontend-assets" uses a
non-deterministic "yarn install"; replace that command with a lockfile-enforced
install: use "yarn install --frozen-lockfile" if the repo uses Yarn v1, or "yarn
install --immutable" if it uses Yarn v2+/Berry; update the
"build-web-frontend-assets" array accordingly and ensure CI uses the same Yarn
major version.
In `@docker-compose.yml`:
- Line 4: Remove the hard-coded personal registry image tag currently set via
the image: key in docker-compose (the "tatevikg1/phplist:test" entry) and
replace it with a project-local tag or omit the image: line so the service uses
its local build context (build: .); update the compose service definition that
contains the image: key to either use a neutral project image name (e.g.,
phplist:local) or delete the image: line entirely to rely on the existing build:
. configuration.
In `@tests/System/HttpEndpoints/WebFrontEndTest.php`:
- Line 49: The current weak assertion in testHomepageReturnsContent uses
assertNotEquals(500, $response->getStatusCode()) which allows other error codes;
update the assertion to assert a successful HTTP result instead — e.g. replace
the not-equals check with a positive check such as asserting
$response->isSuccessful() (or assertEquals(200, $response->getStatusCode()) or
assertThat the status is in the 2xx range) so the test in
testHomepageReturnsContent reliably verifies successful content delivery.
---
Outside diff comments:
In @.gitignore:
- Line 1: Remove the repository-wide wildcard entry '*' from .gitignore (the
root wildcard that ignores all new files) and replace it with either specific
ignore patterns (e.g., editor backups, build artifacts, OS files) or move broad
personal ignores to a global git exclude via git config --global
core.excludesfile; if you intended to ignore everything except a few files,
convert to explicit negation rules instead of '*'. Ensure you update the
existing .gitignore entry that currently contains '*' to one of these safer
options so normal staging and subsequent ignore rules work correctly.
In `@Dockerfile`:
- Around line 47-50: The Dockerfile runs "composer install" before "COPY . .",
which triggers Composer post-install scripts that call "php bin/console" when
bin/console doesn't exist yet; either move the "COPY . ." step so that the
application files (including bin/console) are copied before running "composer
install", or change the composer step to "composer install --no-scripts --no-dev
--no-interaction --prefer-dist --optimize-autoloader" and then run "composer run
scripts" (or "composer install" without --no-scripts) after the COPY so
post-install scripts can find bin/console.
---
Nitpick comments:
In `@postcss.config.js`:
- Around line 2-5: The plugins array in postcss.config.js includes
require('autoprefixer') which is redundant with Tailwind v4's
require('`@tailwindcss/postcss`'); remove the require('autoprefixer') entry from
the plugins array so only require('`@tailwindcss/postcss`') remains, and ensure
the plugins array syntax remains valid after removal.
🪄 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: 40e54fb2-fbc9-4018-9810-9466038bdea5
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.dockerignore.gitignoreDockerfileREADME.mdcomposer.jsondocker-compose.ymlpackage.jsonphpunit.xml.distpostcss.config.jstests/System/HttpEndpoints/WebFrontEndTest.phptests/bootstrap.phpwebpack.config.js
Comment on lines
+94
to
+97
| "build-web-frontend-assets": [ | ||
| "yarn install", | ||
| "yarn build:web-frontend" | ||
| ], |
There was a problem hiding this comment.
Make frontend dependency installation deterministic.
Using plain yarn install can produce drift across image builds. Enforce lockfile-based install in this script.
🔒 Proposed change
"build-web-frontend-assets": [
- "yarn install",
+ "yarn install --frozen-lockfile --non-interactive",
"yarn build:web-frontend"
],For the Yarn version used here, what is the correct deterministic CI install flag (`--frozen-lockfile` vs `--immutable`)?
🤖 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 `@composer.json` around lines 94 - 97, The build script
"build-web-frontend-assets" uses a non-deterministic "yarn install"; replace
that command with a lockfile-enforced install: use "yarn install
--frozen-lockfile" if the repo uses Yarn v1, or "yarn install --immutable" if it
uses Yarn v2+/Berry; update the "build-web-frontend-assets" array accordingly
and ensure CI uses the same Yarn major version.
| app: | ||
| build: . | ||
| image: phplist/base-distribution:latest | ||
| image: tatevikg1/phplist:test |
There was a problem hiding this comment.
Avoid committing a personal registry image tag in default compose config.
Keeping tatevikg1/phplist:test here can accidentally pull/use an external personal image in shared workflows. Prefer a project-local tag or omit image for default local builds.
🔧 Proposed change
- image: tatevikg1/phplist:test
+ image: phplist/base-distribution:localor remove the image: line entirely and rely on build: ..
📝 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
| image: tatevikg1/phplist:test | |
| image: phplist/base-distribution:local |
🤖 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 `@docker-compose.yml` at line 4, Remove the hard-coded personal registry image
tag currently set via the image: key in docker-compose (the
"tatevikg1/phplist:test" entry) and replace it with a project-local tag or omit
the image: line so the service uses its local build context (build: .); update
the compose service definition that contains the image: key to either use a
neutral project image name (e.g., phplist:local) or delete the image: line
entirely to rely on the existing build: . configuration.
| { | ||
| self::getClient()->request('get', '/api/v2'); | ||
| $response = self::getClient()->getResponse(); | ||
| self::assertNotEquals(500, $response->getStatusCode()); |
There was a problem hiding this comment.
Weak assertion allows non-success status codes.
assertNotEquals(500, ...) passes for any non-500 status, including 404, 403, or other error codes. The test name testHomepageReturnsContent suggests it should verify successful content delivery, not just the absence of server errors.
Consider using a positive assertion:
✅ Proposed fix to assert success status
- self::assertNotEquals(500, $response->getStatusCode());
+ self::assertSame(Response::HTTP_OK, $response->getStatusCode());Alternatively, if this test is meant to be distinct from testHomepageReturnsSuccess, consider asserting a 2xx range or removing the status check entirely if assertNotEmpty($response->getContent()) is sufficient.
📝 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
| self::assertNotEquals(500, $response->getStatusCode()); | |
| self::assertSame(Response::HTTP_OK, $response->getStatusCode()); |
🤖 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 `@tests/System/HttpEndpoints/WebFrontEndTest.php` at line 49, The current weak
assertion in testHomepageReturnsContent uses assertNotEquals(500,
$response->getStatusCode()) which allows other error codes; update the assertion
to assert a successful HTTP result instead — e.g. replace the not-equals check
with a positive check such as asserting $response->isSuccessful() (or
assertEquals(200, $response->getStatusCode()) or assertThat the status is in the
2xx range) so the test in testHomepageReturnsContent reliably verifies
successful content delivery.
* release: dev → main (add frontend build system) (#110) New Features Modern frontend build and bundling setup with Node/Yarn support and asset compilation CORS support enabled for API compatibility Chores Updated build tooling and runtime dependencies; added image processing support in the runtime environment Docker and compose improvements for build steps and persistent logs/cache; adjusted ignore rules Tests Added test bootstrap and improved server-error assertion in endpoint tests Documentation Added instructions to build and publish frontend assets --------- Co-authored-by: Tatevik <tatevikg1@gmail.com> * Add dockerhub publish workflow * Fix docker compose configs * Update docker-publish workflow to trigger on main and dev branches * Add todo * Update tags strategy in docker-publish workflow --------- Co-authored-by: Tatevik <tatevikg1@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/docker-publish.yml (1)
14-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove CI publish target off a personal DockerHub namespace.
Line 14 points releases to a personal repo; this is fragile for org-owned publishing and release provenance.
Suggested fix
- DOCKERHUB_IMAGE: tatevikg1/phplist4 + DOCKERHUB_IMAGE: phplist/base-distribution🤖 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 @.github/workflows/docker-publish.yml at line 14, The CI is publishing to a personal DockerHub namespace via the DOCKERHUB_IMAGE variable; change the value of DOCKERHUB_IMAGE to point at the organisation-owned image (or a generic org repository) instead of "tatevikg1/phplist4", and update any related workflow references to use that variable; if necessary, coordinate with org owners to use the org DockerHub repository name or a repository dispatch/secret (DOCKERHUB_IMAGE) that references the canonical org image for releases.docker-compose.yml (1)
5-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a neutral default image tag in shared compose config.
Line 5 still hard-codes a personal registry image, which can cause teams/CI to pull unintended images by default.
Suggested fix
- image: tatevikg1/phplist:test + image: phplist/base-distribution:local🤖 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 `@docker-compose.yml` at line 5, The docker-compose.yml currently hard-codes a personal registry image "tatevikg1/phplist:test"; change this to a neutral default image tag (e.g., replace the image value with a team/official or generic tag such as "phplist:latest" or remove the image key and use a build/context reference) so shared configs and CI do not pull a personal image by default; update the image field in docker-compose.yml accordingly and ensure any related service name remains unchanged.
🤖 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 @.github/workflows/docker-publish.yml:
- Line 23: The workflow currently uses floating tags for actions (e.g.,
actions/checkout, docker/setup-qemu-action, docker/setup-buildx-action,
docker/login-action, docker/metadata-action, docker/build-push-action); replace
each `uses:` entry with the corresponding immutable commit SHA (pin to a full
commit ref like actions/checkout@<SHA>) to reduce supply-chain risk, updating
all occurrences of those symbols in the workflow file (the instances referenced
in the review) so none remain on major/minor tags.
In `@docker-compose.yml`:
- Around line 17-19: The docker-compose env values REST_API_BASE_URL,
FRONT_END_BASE_URL and API_BASE_URL currently point to the internal Docker
hostname "http://app:8081/" which browsers cannot resolve; change them to a
host-reachable URL by replacing "app" with a public host/port variable (for
example use an interpolated env var like ${PUBLIC_HOST}:${PHPLIST_PORT} or
${HOST}:${PHPLIST_PORT} or a full external URL) so the browser-facing base URLs
resolve outside the Docker network; update the three keys (REST_API_BASE_URL,
FRONT_END_BASE_URL, API_BASE_URL) to reference the public-facing environment
variable(s) instead of the internal service name.
---
Duplicate comments:
In @.github/workflows/docker-publish.yml:
- Line 14: The CI is publishing to a personal DockerHub namespace via the
DOCKERHUB_IMAGE variable; change the value of DOCKERHUB_IMAGE to point at the
organisation-owned image (or a generic org repository) instead of
"tatevikg1/phplist4", and update any related workflow references to use that
variable; if necessary, coordinate with org owners to use the org DockerHub
repository name or a repository dispatch/secret (DOCKERHUB_IMAGE) that
references the canonical org image for releases.
In `@docker-compose.yml`:
- Line 5: The docker-compose.yml currently hard-codes a personal registry image
"tatevikg1/phplist:test"; change this to a neutral default image tag (e.g.,
replace the image value with a team/official or generic tag such as
"phplist:latest" or remove the image key and use a build/context reference) so
shared configs and CI do not pull a personal image by default; update the image
field in docker-compose.yml accordingly and ensure any related service name
remains unchanged.
🪄 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: 34d6192e-060d-4004-b699-d83a25572ee5
📒 Files selected for processing (5)
.github/workflows/docker-publish.ymldocker-compose.ymldocker/apache/000-default.confdocker/apache/ports.confdocker/apache/servername.conf
✅ Files skipped from review due to trivial changes (3)
- docker/apache/ports.conf
- docker/apache/servername.conf
- docker/apache/000-default.conf
| contents: read | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all `uses:` entries in workflows and find non-SHA refs.
rg -n --type=yaml '^\s*uses:\s*[^@]+@' .github/workflows \
| rg -n -v '@[0-9a-fA-F]{40}$'Repository: phpList/base-distribution
Length of output: 798
🏁 Script executed:
cat -n .github/workflows/docker-publish.yml | head -60Repository: phpList/base-distribution
Length of output: 1947
Pin GitHub Actions to immutable SHAs instead of floating version tags.
Using floating major tags for build/publish actions increases supply-chain risk. Pin all uses: entries to full commit SHAs.
Lines 23, 26, 29, 33, 40, 50 currently use:
actions/checkout@v4docker/setup-qemu-action@v3docker/setup-buildx-action@v3docker/login-action@v3docker/metadata-action@v5docker/build-push-action@v6
Replace with specific SHAs (e.g., actions/checkout@a5ac7e51b41094c5405359c7b978efb3c90df8be).
🤖 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 @.github/workflows/docker-publish.yml at line 23, The workflow currently uses
floating tags for actions (e.g., actions/checkout, docker/setup-qemu-action,
docker/setup-buildx-action, docker/login-action, docker/metadata-action,
docker/build-push-action); replace each `uses:` entry with the corresponding
immutable commit SHA (pin to a full commit ref like actions/checkout@<SHA>) to
reduce supply-chain risk, updating all occurrences of those symbols in the
workflow file (the instances referenced in the review) so none remain on
major/minor tags.
Comment on lines
+17
to
+19
| REST_API_BASE_URL: 'http://app:8081/' | ||
| FRONT_END_BASE_URL: 'http://app:8081/' | ||
| API_BASE_URL: 'http://app:8081/' |
There was a problem hiding this comment.
Avoid internal-only hostname for browser-facing base URLs.
Line 17-19 use http://app:8081/; that hostname is Docker-network internal and is usually not resolvable by end-user browsers. Prefer host-reachable URLs (for example via ${PHPLIST_PORT} / external host env).
Suggested fix
- REST_API_BASE_URL: 'http://app:8081/'
- FRONT_END_BASE_URL: 'http://app:8081/'
- API_BASE_URL: 'http://app:8081/'
+ REST_API_BASE_URL: ${REST_API_BASE_URL:-http://localhost:${PHPLIST_PORT:-8081}/}
+ FRONT_END_BASE_URL: ${FRONT_END_BASE_URL:-http://localhost:${PHPLIST_PORT:-8081}/}
+ API_BASE_URL: ${API_BASE_URL:-http://localhost:${PHPLIST_PORT:-8081}/}📝 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
| REST_API_BASE_URL: 'http://app:8081/' | |
| FRONT_END_BASE_URL: 'http://app:8081/' | |
| API_BASE_URL: 'http://app:8081/' | |
| REST_API_BASE_URL: ${REST_API_BASE_URL:-http://localhost:${PHPLIST_PORT:-8081}/} | |
| FRONT_END_BASE_URL: ${FRONT_END_BASE_URL:-http://localhost:${PHPLIST_PORT:-8081}/} | |
| API_BASE_URL: ${API_BASE_URL:-http://localhost:${PHPLIST_PORT:-8081}/} |
🤖 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 `@docker-compose.yml` around lines 17 - 19, The docker-compose env values
REST_API_BASE_URL, FRONT_END_BASE_URL and API_BASE_URL currently point to the
internal Docker hostname "http://app:8081/" which browsers cannot resolve;
change them to a host-reachable URL by replacing "app" with a public host/port
variable (for example use an interpolated env var like
${PUBLIC_HOST}:${PHPLIST_PORT} or ${HOST}:${PHPLIST_PORT} or a full external
URL) so the browser-facing base URLs resolve outside the Docker network; update
the three keys (REST_API_BASE_URL, FRONT_END_BASE_URL, API_BASE_URL) to
reference the public-facing environment variable(s) instead of the internal
service name.
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