-
-
Notifications
You must be signed in to change notification settings - Fork 17
Switch to pnpm #1160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch to pnpm #1160
Conversation
|
Warning Rate limit exceeded@DTTerastar has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 3 seconds before requesting another review. ⌛ 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. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes update build and development scripts from using the npm package manager to pnpm in the project configuration, Playwright test configuration, CI workflow, Dockerfile, and build instructions. All related commands and informational messages referencing npm have been replaced with their pnpm equivalents. Node.js 20 setup was added to CI, and pnpm installation was added to the Dockerfile. The Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant ProjectConfig
participant PlaywrightConfig
participant CIWorkflow
participant DockerBuild
Developer->>ProjectConfig: Start/Build/Publish Project
ProjectConfig->>ProjectConfig: Use pnpm install/build/start (was npm)
Developer->>PlaywrightConfig: Run Playwright tests
PlaywrightConfig->>PlaywrightConfig: Use pnpm run build & pnpm run preview (was npm)
CIWorkflow->>CIWorkflow: Setup Node.js 20 and enable pnpm before .NET restore
DockerBuild->>DockerBuild: Install Node.js, then globally install pnpm
Possibly related PRs
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/ESPresense.Companion.csproj(2 hunks)src/ui/playwright.config.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Deploy to Docker
src/ESPresense.Companion.csproj
[error] 64-64: The command "pnpm install" exited with code 127. Error: pnpm not found during dotnet publish.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
src/ui/playwright.config.ts (1)
5-5:Details
❓ Verification inconclusive
LGTM! Verify pnpm availability in test environment.
The command change from npm to pnpm is consistent with the package manager migration. However, ensure that pnpm is installed in the environment where Playwright tests run.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any CI/test configuration files that need to install pnpm # Expected: Find workflow files or Docker configurations that may need pnpm installation echo "Searching for CI/workflow files that may need pnpm installation..." fd -e yml -e yaml -e json | xargs grep -l "npm\|node" | head -10 echo -e "\nSearching for Docker files that may need pnpm installation..." fd -e dockerfile -e Dockerfile | head -5 echo -e "\nSearching for package.json to verify pnpm workspace configuration..." fd package.json | head -5Length of output: 658
Ensure pnpm is installed in CI/test environments
Please confirm that pnpm is available where Playwright tests execute:
- src/ui/playwright.config.ts: updated to
command: 'pnpm run build && pnpm run preview',- CI workflows (
.github/workflows/*.yml): include pnpm setup (e.g.actions/setup-pnpmornpm install -g pnpm).- Dockerfiles (if any): install pnpm globally.
- src/config.example.yaml and other sample configs: replace any
npm runreferences withpnpm run.src/ESPresense.Companion.csproj (2)
13-13: LGTM! Consistent with pnpm migration.The SPA proxy launch command change is appropriate for the package manager migration.
58-60: LGTM! Clear messaging and correct command.The updated message and pnpm install command are consistent with the migration goals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(1 hunks)BUILDING.md(2 hunks)Dockerfile(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/ci.yml
- BUILDING.md
🧰 Additional context used
🪛 GitHub Actions: Deploy to Docker
Dockerfile
[warning] 24-24: Dockerfile warning: Usage of undefined variable '$TARGETPLATFORM'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
28-28: Remove trailing whitespaceYAML lint flagged a trailing space on this line. Removing it will satisfy lint rules.
- working-directory: src/ui + working-directory: src/ui🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 28-28: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 28-28: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
27-27: Verify UI directory pathEnsure that the
src/uidirectory exists at the repository root. The pnpm setup step will fail if this path is incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
22-27:⚠️ Potential issueSwap pnpm and Node.js setup steps
pnpm/action-setup is executed before Node.js is installed, which will cause failures. Move the Node.js setup step above the pnpm setup and ensure consistent ordering ofnamebeforeuses.Apply this diff:
@@ jobs.build.steps - - uses: pnpm/action-setup@v4 - name: Install pnpm + - name: Install Node.js + uses: actions/setup-node@v4 + with: + node-version: 20 + cache: 'pnpm' + + - uses: pnpm/action-setup@v4 + name: Install pnpm
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/ci.yml
34-34: input "working-directory" is not defined in action "actions/setup-node@v4". available inputs are "always-auth", "architecture", "cache", "cache-dependency-path", "check-latest", "node-version", "node-version-file", "registry-url", "scope", "token"
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 28-28: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
22-34: Reorder Node.js and pnpm setup steps; align key ordering
pnpm requires Node.js to be available before installation. Move the Install Node.js step above Install pnpm, and listnamebeforeusesin both steps for consistency.@@ - - uses: pnpm/action-setup@v4 - name: Install pnpm - with: - version: 10 - run_install: false - working-directory: src/ui + - name: Install Node.js + uses: actions/setup-node@v4 + with: + node-version: 20 + cache: 'pnpm' + cache-dependency-path: src/ui + + - name: Install pnpm + uses: pnpm/action-setup@v4 + with: + version: 10 + run_install: false + working-directory: src/ui🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 28-28: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
28-28: Remove trailing whitespace
A trailing space on this empty line can cause lint errors.-🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 28-28: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 28-28: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
22-28:⚠️ Potential issueReorder Node.js and pnpm setup and standardize key order
pnpm/action-setup requires Node.js to be present first; please move the Node.js setup step (lines 29–34) above the pnpm setup. Also, place
name:beforeuses:for consistency.Proposed diff:
@@ jobs.build.steps - - uses: pnpm/action-setup@v4 - name: Install pnpm + - name: Install Node.js + uses: actions/setup-node@v4 + with: + node-version: 20 + cache: 'pnpm' + cache-dependency-path: src/ui/pnpm-lock.yaml + + - name: Install pnpm + uses: pnpm/action-setup@v4 with: version: 10 run_install: false working-directory: src/ui🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 28-28: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
22-28: Remove trailing spacesLine 28 contains trailing whitespace flagged by YAMLlint. Please remove it.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 28-28: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 28-28: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Deploy to Docker add-on with tag
- GitHub Check: Analyze (csharp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
22-27:⚠️ Potential issueCritical: Ensure Node.js is installed before pnpm
pnpm requires a Node.js runtime and will error out if Node isn't available. This job currently adds pnpm without any preceding
actions/setup-nodestep. Either remove this pnpm setup if it isn’t used here, or insert a Node.js setup step immediately before it.Proposed diff:
@@ jobs.build.steps - - uses: pnpm/action-setup@v4 - name: Install pnpm - with: - version: 10 - run_install: false + - name: Install Node.js + uses: actions/setup-node@v4 + with: + node-version: 20 + cache: 'pnpm' + + - name: Install pnpm + uses: pnpm/action-setup@v4 + with: + version: 10 + run_install: false🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 27-27: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
22-23: Nitpick: Consistent key orderingMost steps list
namebeforeuses. Swap these two lines for the pnpm step to match the existing style.@@ - - uses: pnpm/action-setup@v4 - name: Install pnpm + - name: Install pnpm + uses: pnpm/action-setup@v4
27-27: Nitpick: Remove trailing whitespaceYAML lint flags trailing spaces on this line. Please delete the extra spaces at end-of-line.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 27-27: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/ui/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)src/ui/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/ui/package.json
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 27-27: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Deploy to Docker add-on with tag
- GitHub Check: Analyze (csharp)
Summary
pnpminstead ofnpmin build scriptspackage-lock.jsonfilesTesting
dotnet test -c Release --no-build --verbosity normal --filter "Category!=LongRunning"(fails: dotnet not installed)https://chatgpt.com/codex/tasks/task_e_683fb5d095048324bff8d21699bd8aa8
Summary by CodeRabbit