feat: migrate frontend pnpm to v11#4785
Conversation
Signed-off-by: Muhammad Hassaan Saleem <[email protected]>
|
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:
WalkthroughUpgrade frontend to pnpm v11, move PNPM settings into workspace config, update Dockerfiles to use PNPM fetch retry/timeout and PNPM_HOME, and adjust Docker build COPY sets to include framework config files while removing ChangesPNPM v11 Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🤖 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 `@docker/frontend/Dockerfile.a11y.test`:
- Around line 8-10: The ENV block uses PNPM_CONFIG_NETWORK_RETRY_COUNT and
PNPM_CONFIG_NETWORK_TIMEOUT which are not the documented pnpm request variables;
update the ENV declaration that sets PNPM_HOME on the PATH to replace
PNPM_CONFIG_NETWORK_RETRY_COUNT and PNPM_CONFIG_NETWORK_TIMEOUT with
PNPM_CONFIG_FETCH_RETRIES=5 and PNPM_CONFIG_FETCH_TIMEOUT=30000 so pnpm's
fetchRetries/fetchTimeout are honored (leave PNPM_HOME and PATH handling
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: ASSERTIVE
Plan: Pro
Run ID: 60fe8e7f-2036-4160-9dd2-8008724ad7c5
📒 Files selected for processing (6)
docker/frontend/Dockerfile.a11y.testdocker/frontend/Dockerfile.localdocker/frontend/Dockerfile.unit.testfrontend/.pnpmrcfrontend/package.jsonfrontend/pnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- frontend/.pnpmrc
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 6 files
Confidence score: 3/5
- There is a concrete reliability risk: in
docker/frontend/Dockerfile.unit.testanddocker/frontend/Dockerfile.a11y.test,PNPM_CONFIG_NETWORK_RETRY_COUNTandPNPM_CONFIG_NETWORK_TIMEOUTappear to be invalid for pnpm v11, so retry/timeout tuning may silently not apply. docker/frontend/Dockerfile.localhas the same configuration-pattern issue for pnpm fetch retry/timeout, which raises regression risk for local/dev build stability under flaky networks.- Given the medium-high severity (6–7/10) and high confidence, this is not a hard blocker but does add meaningful merge risk until the env var names are corrected.
- Pay close attention to
docker/frontend/Dockerfile.unit.test,docker/frontend/Dockerfile.a11y.test,docker/frontend/Dockerfile.local- pnpm network retry/timeout env vars may be no-ops due to incorrect key names.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Signed-off-by: hassaansaleem28 <[email protected]>
hassaansaleem28
dismissed stale reviews from cubic-dev-ai[bot] and coderabbitai[bot]
via
416ee74
Signed-off-by: hassaansaleem28 <[email protected]>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Signed-off-by: hassaansaleem28 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docker/frontend/Dockerfile.a11y-tests`:
- Around line 8-13: The Dockerfile sets PNPM_CONFIG_FETCH_RETRIES,
PNPM_CONFIG_FETCH_TIMEOUT and disables PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN which
avoids lockfile verification at runtime; ensure lockfile integrity by either
enabling verification during the build step that runs pnpm install (e.g., use a
frozen lockfile or run `pnpm install --frozen-lockfile` in the earlier install
step) or move verification to a build stage that runs as root/with a TTY; update
the Dockerfile build steps around the pnpm install phase to enforce lockfile
validation while keeping PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN=false for the
non-root test runtime.
In `@docker/frontend/Dockerfile.local`:
- Around line 41-43: The Dockerfile sets ENV
PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN=false to disable pnpm v11's
verifyDepsBeforeRun behavior; scope this env var so it only applies to the
development image/workflow (e.g., keep it in Dockerfile.local or the dev stage)
and add a short comment next to the ENV line explaining the integrity tradeoff
and that package integrity is still enforced during the install/build stage
using trusted inputs and lockfile verification (i.e., document that this setting
avoids runtime verification but does not skip install-time integrity checks).
In `@docker/frontend/Dockerfile.unit-tests`:
- Around line 8-13: The Docker test images currently disable pnpm's dependency
verification by setting PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN=false; remove that
override (or set it to true) so pnpm will validate node_modules against
pnpm-lock.yaml, and keep PNPM_CONFIG_FETCH_RETRIES and PNPM_CONFIG_FETCH_TIMEOUT
as-is; make the same change in the counterpart Dockerfile (the a11y-tests
variant) and if EACCES during `pnpm run` was the reason for disabling
verification, instead fix the underlying permission/installation flow in the
image (run installs as the same user who will run tests, preserve node_modules
from the build stage, or adjust ownership) rather than suppressing
PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 79248849-de38-438e-8856-88cabd7124b5
📒 Files selected for processing (4)
docker/frontend/Dockerfile.a11y-testsdocker/frontend/Dockerfile.localdocker/frontend/Dockerfile.unit-testsfrontend/pnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- frontend/pnpm-workspace.yaml
| # Use the deps installed in the image; don't let pnpm v11 re-verify and try to | ||
| # purge node_modules before `pnpm run` (non-root user, no TTY -> EACCES). | ||
| ENV PATH="$PNPM_HOME:$PATH" \ | ||
| PNPM_CONFIG_FETCH_RETRIES=5 \ | ||
| PNPM_CONFIG_FETCH_TIMEOUT=30000 \ | ||
| PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN=false |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
pnpm v11 environment variables PNPM_CONFIG_FETCH_RETRIES PNPM_CONFIG_FETCH_TIMEOUT PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN
💡 Result:
In pnpm v11, configuration is managed through specific files (such as pnpm-workspace.yaml or a global config.yaml) or via environment variables [1][2]. pnpm now explicitly requires the use of the pnpm_config_ prefix for environment variables; it no longer reads npm_config_* variables [1][2]. Environment variables starting with pnpm_config_ (case-insensitive) are loaded into the configuration and override settings defined in configuration files, though they do not override command-line arguments [1]. The specific environment variables you inquired about are used as follows: PNPM_CONFIG_FETCH_RETRIES This corresponds to the fetchRetries setting [3]. It defines the number of retries for network requests when fetching packages from the registry (default: 2) [3]. PNPM_CONFIG_FETCH_TIMEOUT This corresponds to the fetchTimeout setting [3]. It defines the timeout (in milliseconds) for network requests when fetching packages [3]. PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN This corresponds to the verifyDepsBeforeRun setting [3]. It allows for the verification of the state of dependencies before executing scripts with commands like pnpm run or pnpm exec [3]. Supported values include install, warn, error, prompt, or false [3]. Note that because pnpm v11 has deprecated the use of npm_config_* environment variables, you must update any existing CI/CD pipelines, shell scripts, or environment configurations to use the pnpm_config_ prefix to ensure they are correctly recognized by pnpm [1][2][4]. Settings should ideally be moved to pnpm-workspace.yaml (for project-specific config) or the global config.yaml (for global settings) where possible [1][3][2].
Citations:
- 1: https://pnpm.io/configuring
- 2: https://pnpm.io/blog/releases/11.0
- 3: https://pnpm.io/settings
- 4: NPM 11 complains about extraneous (PNPM) settings inside ~/.npmrc pnpm/pnpm#9232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant ENV block in Dockerfile.unit-tests
echo "=== docker/frontend/Dockerfile.unit-tests (lines 1-60) ==="
sed -n '1,60p' docker/frontend/Dockerfile.unit-tests
# Find a11y-tests Dockerfile and show relevant section
echo
echo "=== Candidate a11y Dockerfiles under docker/frontend ==="
fd -t f 'Dockerfile.*a11y*' docker/frontend -H || true
echo
if [ -f docker/frontend/Dockerfile.a11y-tests ]; then
echo "=== docker/frontend/Dockerfile.a11y-tests (lines 1-60) ==="
sed -n '1,60p' docker/frontend/Dockerfile.a11y-tests
else
echo "docker/frontend/Dockerfile.a11y-tests not found; listing candidates above only."
fi
# Also search for the specific env var name in the docker/frontend dir to see all occurrences.
echo
echo "=== Occurrences of PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN in docker/frontend ==="
rg -n "PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN" docker/frontend || trueRepository: OWASP/Nest
Length of output: 2683
Revisit disabling pnpm dependency verification in test images
PNPM_CONFIG_FETCH_RETRIES, PNPM_CONFIG_FETCH_TIMEOUT, and PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN=false use the correct pnpm v11 pnpm_config_* environment-variable mappings (case-insensitive) and are set identically in both docker/frontend/Dockerfile.unit-tests and docker/frontend/Dockerfile.a11y-tests.
Disabling verifyDepsBeforeRun skips pnpm’s check that the installed node_modules matches pnpm-lock.yaml before pnpm run, which can hide dependency/lockfile drift (even if you pin with --frozen-lockfile during the image build).
🤖 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/frontend/Dockerfile.unit-tests` around lines 8 - 13, The Docker test
images currently disable pnpm's dependency verification by setting
PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN=false; remove that override (or set it to
true) so pnpm will validate node_modules against pnpm-lock.yaml, and keep
PNPM_CONFIG_FETCH_RETRIES and PNPM_CONFIG_FETCH_TIMEOUT as-is; make the same
change in the counterpart Dockerfile (the a11y-tests variant) and if EACCES
during `pnpm run` was the reason for disabling verification, instead fix the
underlying permission/installation flow in the image (run installs as the same
user who will run tests, preserve node_modules from the build stage, or adjust
ownership) rather than suppressing PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN.
Signed-off-by: hassaansaleem28 <[email protected]>
|
@coderabbitai review this pull |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== frontend/pnpm-workspace.yaml ==="
cat frontend/pnpm-workspace.yaml
echo ""
echo "=== frontend/package.json ==="
cat frontend/package.json
echo ""
echo "=== frontend/.pnpmrc ==="
cat frontend/.pnpmrc
echo ""
echo "=== docker/frontend/Dockerfile ==="
cat docker/frontend/Dockerfile
echo ""
echo "=== docker/frontend/Dockerfile.local ==="
cat docker/frontend/Dockerfile.local
echo ""
echo "=== docker/frontend/Dockerfile.unit-tests ==="
cat docker/frontend/Dockerfile.unit-tests
echo ""
echo "=== docker/frontend/Dockerfile.a11y-tests ==="
cat docker/frontend/Dockerfile.a11y-testsLength of output: 16502 The PR description image shows a terminal output with all pre-commit checks passing — CSpell (1829 files, 0 issues), hadolint, yamllint, ruff, mypy, pnpm run lint, pnpm run graphql-codegen, and 30+ other checks — all marked green "Passed". After reviewing the file contents, here is my full assessment: Review of PR
|
| Item | Severity | Status |
|---|---|---|
Production Dockerfile missing PNPM_CONFIG_FETCH_RETRIES/PNPM_CONFIG_FETCH_TIMEOUT |
🔴 Medium | Needs fix |
| PR description env var names don't match implementation | Update description | |
Config migration from .pnpmrc/package.json → pnpm-workspace.yaml |
✅ | Correct |
allowBuilds list |
✅ | Appropriate |
PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN=false usage |
✅ | Well-justified |
| All pre-commit checks passing | ✅ | Confirmed |
The overall approach is solid. Please address the production Dockerfile inconsistency before merging.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Could you also cover other cases where pnpm is used?
| # Deps are installed at build time with `pnpm install --frozen-lockfile` (which | ||
| # enforces lockfile integrity), so skip pnpm v11's pre-run re-verification -- | ||
| # otherwise `pnpm run` purges/reinstalls the image's node_modules as the | ||
| # non-root user (no TTY -> EACCES). |
There was a problem hiding this comment.
This comment is too long for a single line of code. Please make it shorter and use full length words (I mean the deps case)
| # "unrs" (from "unrs-resolver", a native build dependency allow-listed above | ||
| # for pnpm v11) is not a dictionary word; accept it here instead of adding it | ||
| # to the shared cSpell dictionary. | ||
| # cspell:ignore unrs |
There was a problem hiding this comment.
We don't use in file ignores. Moreover the comment again has 4 lines to explain 1 line of code.
Signed-off-by: hassaansaleem28 <[email protected]>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4785 +/- ##
=======================================
Coverage 98.88% 98.88%
=======================================
Files 538 538
Lines 17143 17143
Branches 2405 2457 +52
=======================================
Hits 16951 16951
Misses 103 103
Partials 89 89
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
cspell and e2e also use pnpm
2 participants
Proposed change
Resolves #4747
Upgrades pnpm from v10.33.3 to v11.4.0 for the frontend.
(PNPM_CONFIG_NETWORK_RETRY_COUNT and PNPM_CONFIG_NETWORK_TIMEOUT) since
v11 dropped npm_config_* passthrough for its own config.
make check passes -- also app runs without error logs
Checklist
make check-testlocally: all warnings addressed, tests passed