Conversation
feat: release/2.6.5
There was a problem hiding this comment.
🔵 Needs a closer look
Broad toolchain changes require additional validation of front-end widget and consent behavior.
Pull request overview
Patches 58 Dependabot alerts in build-only dependencies and GitHub Actions without changing shipped PHP dependencies.
Changes:
- Upgrades vulnerable Yarn dependencies and adds four targeted resolutions.
- Updates
setup-phpin deployment workflows. - Leaves two documented local-tooling alerts unresolved.
File summaries
| File | Summary |
|---|---|
yarn.lock |
Updates vulnerable build dependencies and checksums. |
package.json |
Adds dependency resolutions. |
.github/workflows/deploy.yml |
Updates the PHP setup action. |
.github/workflows/deploy-demo.yml |
Updates the PHP setup action. |
Review details
- Files reviewed: 3/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2bf9d06 to
587f3a6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved Vite asset-copying, GitLab CI compatibility, and deployment-documentation issues remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
vite.config.mjs:11
- These
cpSynccalls do not add the static directories to Rollup's watch graph.yarn starttherefore does not rebuild when a PHP-only asset such asassets/img/icon.pngorlogo.svgchanges, leaving the correspondingdistcopy stale until another entry rebuild or a production build. Register the static files/directories withthis.addWatchFileor use a copy plugin with watch support.
cpSync('assets/img', 'dist/img', { recursive: true });
cpSync('assets/fonts', 'dist/fonts', { recursive: true });
vite.config.mjs:9
closeBundleis not run after each Rollup watch rebuild; it is deferred until the watcher shuts down. A clean checkout followed byyarn starttherefore never copiesdist/imgordist/fonts, so the admin icon and font/image references are missing during development. UsewriteBundlefor this copy (and watch the static directories if live changes are expected).
closeBundle() {
- Files reviewed: 13/15 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate findings remain for the legacy GitLab CI job and packaging exclusions.
Review details
Suppressed comments (2)
docs/deployment/improvements.md:46
- This table is explicitly the historical record for PRs #47/#48/#50, but these rows now attribute this PR's Node 24 and setup-php 2.37.2 upgrades to those older PRs. Keeping the historical Node 18/2.37.0 values here and documenting the current migration separately avoids making the changelog inaccurate.
| 2 | Workflow | Node.js version mismatch: CI used Node 16; `.nvmrc` and `volta` pin Node 24 | Node 24 now used in CI; Node 16 was EOL since September 2023 |
| 4 | Workflow | Outdated GitHub Actions: `checkout@v2`, `setup-node@v2` | Upgraded: `checkout@v4`, `setup-node@v4`, `setup-php@2.37.2` |
package.json:29
- This new engine floor leaves the repository's
.gitlab-ci.ymlESLint job onnode:16.14.0(and it still invokes the bundled Yarn 1 withoutcorepack enable), so any GitLab pipeline that is still enabled will fail before linting. Update that job to Node 24 with Corepack/Yarn 4, or remove the legacy workflow if it is truly retired.
"node": ">= 22.12.0"
- Files reviewed: 15/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Address the missing PR build check and exclude Playwright artifacts from both release packaging lists.
Review details
Suppressed comments (1)
package.json:44
- This new Vite build is still not exercised by any pull-request workflow:
deploy.ymlruns on tags/manual dispatch anddeploy-demo.ymlruns ondevelop, so a broken install or production build can merge without CI feedback. Please add a PR check covering Corepack, the immutable install, lint, andbuild:production, or make an equivalent required check part of this migration.
"vite": "^8"
- Files reviewed: 15/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
Clears all 60 Dependabot alerts, retires the dead build toolchain they came from, and fixes an asset leak found along the way.
Five commits, reviewable independently.
1.
fix(deps): patch 58 Dependabot security alertsyarn.lock(build toolchain)setup-php)composer.lock(shipped PHP)None of these ever reached merchants:
node_modulesis in.distignore, so nothing from the npm tree ships to WordPress.org. The only npm package bundled for the browser is Alpine.js, which has no alerts.shivammathur/setup-php2.37.0 → 2.37.2 in both deploy workflowsTwo alerts resisted:
elliptic(no patched release exists — 6.6.1 is the latest and is itself vulnerable) andimmutable(capped at 3.x bybrowser-sync@3.0.4, itself the latest release). Both are resolved by the next commit.2.
build: migrate the asset pipeline from Laravel Mix to ViteLaravel Mix has had no release since June 2022 and still pulls
node-libs-browser, the webpack 4 polyfill package — which is exactly whereellipticcame from.browser-syncis what cappedimmutable. Both alerts disappear along with those dependencies.node_modulesresolutionsentries.nvmrc) / 18 (CI)Node had to move: Vite 8 requires
^20.19 || >=22.12, so the previous Node 18 in CI could not have run the build at all.No PHP was touched in this commit. It only changes how
dist/is produced, not how WordPress loads it.Two things worth a close look in
vite.config.mjs:iifefor multi-entry builds (Multiple entry points are not supported when output formats include "umd" or "iife"), so the output is ESM. Loaded throughwp_enqueue_script()as a classic script, its top-level declarations land onwindow— a minified_clobbered WordPress's Underscore and brokewp.mediawith_.isArray is not a function. The bundle is therefore wrapped throughbanner/footer.type="module"would be cleaner but defers execution, and c24c711 (PLG-264) fixed precisely that class of bug: the consent signal arriving after a hardcoded GTM tag had already fired. Deferringaxeptio.jswould have reintroduced it.@tailwind→@importinassets/css/backend/main.css. The CSS spec requires@importto precede any other rule; the directives came first, so the bundler silently dropped every@importand stripped all custom rules (8 KB of buttons, tabs, forms, notices). Rewriting them as@import "tailwindcss/base"keeps cascade order identical.The four entry points mirror how WordPress enqueues them: admin and frontend are separate contexts, and merging them would ship Alpine.js and Tailwind (118 KB) on every public page of every merchant site.
3.
fix(admin): stop loading the plugin stylesheet on every admin pageUnrelated to the migration, found while verifying it — and pre-existing since 2023.
admin_scripts()has been guarded by a screen check since 4177b73;admin_styles()never was.backend/main.css(51 KB) was enqueued on every admin screen — Posts, Media, Settings, Users — for a stylesheet only two pages use.The check now lives in
is_plugin_admin_screen(), called by both, so the two lists cannot drift apart again. It takes the$hook_suffixWordPress already passes toadmin_enqueue_scripts, which removes theget_current_screen()call the old code relied on (and the null guard it required).Verified beforehand that nothing else depends on that stylesheet: the review banner renders through
axeptio/before_main_setting_containerandaxeptio/before_plugin_manager_container, both internal to those two pages, and the plugin's onlyadmin_noticescallback uses WordPress's nativenotice notice-errorclasses.Verification
Everything below was run inside ddev (Linux, Node 24), matching CI.
Build chain
yarn install --immutable— passes, the lockfile is CI-cleanyarn eslint— exit 0yarn build:production—dist/identical to the Mix output except three unused artifacts (*.LICENSE.txt,mix-manifest.json), none referenced by PHP-moz-prefix obsolete since Firefox 51phpcs,phpstan,pestonincludes/core.php— clean.distignore:vite.config.mjs,package.json,yarn.lock,node_modulesexcluded;dist/,includes/,vendor/includedAdmin asset loading — 10 pages swept
Frontend — consent path unchanged
<head>axeptio.jsposition in pagewp-consent-api→ inline →axeptio.jsAxeptio_SDK,_axcb,wp_consent_type, steps, vendors, categoriesdataLayerentriesRendering — Alpine reactive, tab switching works, 502 CSS rules applied,
wp.mediafunctional, review banner intact, images and fonts resolve.4.
docs+build— follow-ups from the Copilot reviewdocs: update the build stack and Node version in the environment matrixand the docs fixes that followed.environments.mdhad been updated butworkflows.mdandimprovements.mdstill documented Node 18 andsetup-php@2.37.0, contradicting the workflow changes in this PR. No stale version or webpack mention is left underdocs/.build: copy the static assets on every bundle write—closeBundlebecomeswriteBundle, which is the correct hook for a copy that must follow every write.Two review points were investigated and are worth recording:
closeBundlenever runs during a watch build, leavingdist/imganddist/fontsmissing after a cleanyarn start. Testing does not support this:rm -rf dist && yarn startproduces both directories (10 images, 1 font). The hook change above was made on correctness grounds, not to fix that.alert.svgalone changes nothing, editing any CSS afterwards refreshes it. The suggested fix does not work here, however:addWatchFilewas implemented both on the directories and file-by-file through a recursivereaddirSync, and neither triggered a rebuild under Vite 8 / Rolldown — the log shows a singlebuilt in. The dead code was removed rather than left in place looking functional, and the limitation is documented in the config. In practice, editing an image requires touching any JS or CSS for the copy to refresh.Impact on the pipelines
Every consumer of the npm scripts was audited, since
dev,productionandhotwere removed along with webpack.deploy.yml(WordPress.org)corepack enable,yarn install --immutable,yarn build:productiondeploy-demo.yml(QA demo)Taskfile.yml(task release)yarn && yarn build:productionexclusions.txtupdated alongside.distignoredocs/deployment/*webpack watchand Node 18One pipeline needs a decision:
.gitlab-ci.yml. Itseslintjob runsimage: node:16.14.0, thenyarn && yarn eslint.That job is already failing today, before this PR:
packageManager: "yarn@4.9.2"was added in 46a3121, and the Yarn 1.22 shipped in the Node 16 image refuses to run on a project declaring it. Yarn 4 itself requires Node >= 18.12, so the image was never going to work.This PR does not make it worse, but it does close the door for good: Vite 8 requires Node
^20.19 || >=22.12. If that pipeline is still wired to anything, the image needs to move tonode:24and the job to runcorepack enablefirst. If it is a leftover from the pre-GitHub days, it should be deleted. I left it untouched — that call is yours.Notes for review
main), so merging intodevelopwill not close the alerts — they clear once this reachesmain.corepack_enable: true(in.ddev/config.yaml, outside this repo). Without it the container falls back to Yarn 1.22 andyarn installfails on thepackageManagerfield. CI already runscorepack enable.node_modulesis shared between host and container through the ddev mount, and Rolldown ships platform-specific native bindings — so runningyarn installon the host then building in the container (or the reverse) breaks. Pick one environment.deploy.ymlruns on tags,deploy-demo.ymlon pushes todevelop. This PR is therefore not exercised by CI before merge. A smallci.yml(install + lint + build on PRs) would close that gap — happy to add it here or separately.main: SDK injection, consent handling, and Google Consent Mode signals.