perf(core): update package manager and fix paths#276
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the pnpm version to 11.1.2, refactors workspace build configurations to use the new allowBuilds field, and adjusts file paths in several package.json files. The prepare-local.ts script was modified to ensure local binaries are correctly linked, though feedback suggests this implementation could cause recursive execution loops and unnecessary overhead in CI environments. Additionally, a safety check was recommended to prevent the script from crashing if the node_modules/.bin directory is missing.
| if (packageBins.some((bin) => !nodeBins.includes(bin))) { | ||
| cp.execSync(sh`pnpm install`, { stdio: "inherit" }); | ||
| } |
There was a problem hiding this comment.
Executing pnpm install from within a script called by the prepare hook (which is itself triggered by pnpm install) can cause recursive execution. While it might terminate after the second run if no more builds are needed, it significantly increases installation time and can lead to lockfile contention. Consider if this automatic re-install is strictly necessary or if there's a more efficient way to ensure binaries are linked, such as a manual step or a more targeted linking script.
| import * as fs from "node:fs"; | ||
| import path from "node:path"; | ||
|
|
||
| if (process.env.CI) process.exit(0); |
There was a problem hiding this comment.
The removal of the process.env.CI check will cause this script to run in CI environments. Since this script triggers builds for workspace packages, it may lead to redundant build steps if your CI pipeline already handles building. It's generally recommended to keep this check to avoid unnecessary overhead in CI.
|
|
||
| const dirname = path.dirname(url.fileURLToPath(import.meta.url)); | ||
| const nodeBinDirectory = path.resolve(dirname, "../../node_modules/.bin/"); | ||
| const nodeBins = await fs.promises.readdir(nodeBinDirectory); |
There was a problem hiding this comment.
The fs.promises.readdir call will throw an error if the node_modules/.bin directory does not exist. It is safer to check for the directory's existence before attempting to read it to prevent the script from crashing.
const nodeBins = fs.existsSync(nodeBinDirectory) ? await fs.promises.readdir(nodeBinDirectory) : [];
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe PR normalizes package distribution file path formats, upgrades PNPM to version 11.1.2 with revised workspace build allowlist configuration, and enhances the local build preparation script with binary entry detection and conditional dependency installation. ChangesBuild and package configuration updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
@stephansama/ai-commit-msg
@stephansama/alfred-kaomoji
@stephansama/astro-iconify-svgmap
@stephansama/auto-readme
@stephansama/catppuccin-jsonresume-theme
@stephansama/catppuccin-opml
@stephansama/catppuccin-rss
@stephansama/catppuccin-typedoc
@stephansama/catppuccin-xsl
@stephansama/eslint-config
create-stephansama-example
@stephansama/find-makefile-targets
@stephansama/github-env
@stephansama/multipublish
@stephansama/prettier-plugin-handlebars
@stephansama/remark-asciinema
@stephansama/single-file
@stephansama/svelte-social-share-links
@stephansama/typed-env
@stephansama/typed-events
@stephansama/typed-nocodb-api
@stephansama/typed-templates
@stephansama/types-github-action-env
@stephansama/types-lhci
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@core/multipublish/package.json`:
- Line 28: The package.json currently points the "types" field at a non-existent
path (./config/schema.d.cts); update the "types" entry to reference the
generated declaration inside dist (e.g., "./dist/schema.d.ts") so consumers can
find the emitted type definitions; edit the package.json "types" field to
"./dist/schema.d.ts" (or the exact generated file name) to match the schema
export ("./dist/schema.json") and the build setting that emits d.ts files.
In `@scripts/src/prepare-local.ts`:
- Around line 47-53: The code calls fs.promises.readdir(nodeBinDirectory)
without handling the case where node_modules/.bin doesn't exist; wrap that call
in a try/catch inside prepare-local (around the nodeBinDirectory/nodeBins logic)
so that on ENOENT (or other expected filesystem errors) you set nodeBins = []
(or fallback to an empty array) and optionally log a warning, and only call
cp.execSync(sh`pnpm install`, ...) when packageBins.some(bin =>
!nodeBins.includes(bin)) remains true; reference the existing symbols
nodeBinDirectory, nodeBins, fs.promises.readdir, packageBins, and cp.execSync
when making the change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 99359456-9f95-4eed-9bbd-a4ef71b4c4f9
📒 Files selected for processing (6)
core/auto-readme/package.jsoncore/multipublish/package.jsonpackage.jsonpnpm-workspace.yamlscripts/src/prepare-local.tsscripts/types/packagejson.d.ts
| "./package.json": "./package.json", | ||
| "./schema": "./dist/schema.mjs", | ||
| "./schema.json": "./config/schema.json" | ||
| "./schema.json": "./dist/schema.json" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify schema.json and schema.d.cts locations in multipublish package
# Check if schema.json exists in dist after build
fd -t f 'schema.json' core/multipublish/
# Check if schema.d.cts exists in config
fd -t f 'schema.d.cts' core/multipublish/
# Check the build configuration to see where schema files are generated
cat core/multipublish/tsdown.config.ts 2>/dev/null || echo "No tsdown config found"Repository: stephansama/packages
Length of output: 1126
🏁 Script executed:
cat -n core/multipublish/package.json | sed -n '25,35p'Repository: stephansama/packages
Length of output: 410
🏁 Script executed:
fd -t f 'schema.d.*' core/multipublish/Repository: stephansama/packages
Length of output: 46
🏁 Script executed:
grep -r "schema.d.cts" core/multipublish/ --include="*.ts" --include="*.js" --include="*.json"Repository: stephansama/packages
Length of output: 131
Update the types field to point to the correct location in dist directory.
The types field on line 30 points to ./config/schema.d.cts, but this file does not exist. Since the schema export now points to ./dist/schema.json and the build is configured with dts: true for the schema entry, the type declaration file should be generated in the dist directory. Update the types field to ./dist/schema.d.ts or the appropriate generated file location to ensure the package exports valid type definitions.
🤖 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 `@core/multipublish/package.json` at line 28, The package.json currently points
the "types" field at a non-existent path (./config/schema.d.cts); update the
"types" entry to reference the generated declaration inside dist (e.g.,
"./dist/schema.d.ts") so consumers can find the emitted type definitions; edit
the package.json "types" field to "./dist/schema.d.ts" (or the exact generated
file name) to match the schema export ("./dist/schema.json") and the build
setting that emits d.ts files.
| const dirname = path.dirname(url.fileURLToPath(import.meta.url)); | ||
| const nodeBinDirectory = path.resolve(dirname, "../../node_modules/.bin/"); | ||
| const nodeBins = await fs.promises.readdir(nodeBinDirectory); | ||
|
|
||
| if (packageBins.some((bin) => !nodeBins.includes(bin))) { | ||
| cp.execSync(sh`pnpm install`, { stdio: "inherit" }); | ||
| } |
There was a problem hiding this comment.
Add error handling for missing node_modules/.bin directory.
Line 49 calls fs.promises.readdir on node_modules/.bin/ without error handling. If the directory doesn't exist (e.g., on a fresh clone before any install), the script will crash with an unhandled promise rejection.
🛡️ Proposed fix to add error handling
const dirname = path.dirname(url.fileURLToPath(import.meta.url));
const nodeBinDirectory = path.resolve(dirname, "../../node_modules/.bin/");
-const nodeBins = await fs.promises.readdir(nodeBinDirectory);
+const nodeBins = fs.existsSync(nodeBinDirectory)
+ ? await fs.promises.readdir(nodeBinDirectory)
+ : [];
if (packageBins.some((bin) => !nodeBins.includes(bin))) {
cp.execSync(sh`pnpm install`, { stdio: "inherit" });
}📝 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.
| const dirname = path.dirname(url.fileURLToPath(import.meta.url)); | |
| const nodeBinDirectory = path.resolve(dirname, "../../node_modules/.bin/"); | |
| const nodeBins = await fs.promises.readdir(nodeBinDirectory); | |
| if (packageBins.some((bin) => !nodeBins.includes(bin))) { | |
| cp.execSync(sh`pnpm install`, { stdio: "inherit" }); | |
| } | |
| const dirname = path.dirname(url.fileURLToPath(import.meta.url)); | |
| const nodeBinDirectory = path.resolve(dirname, "../../node_modules/.bin/"); | |
| const nodeBins = fs.existsSync(nodeBinDirectory) | |
| ? await fs.promises.readdir(nodeBinDirectory) | |
| : []; | |
| if (packageBins.some((bin) => !nodeBins.includes(bin))) { | |
| cp.execSync(sh`pnpm install`, { stdio: "inherit" }); | |
| } |
🤖 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 `@scripts/src/prepare-local.ts` around lines 47 - 53, The code calls
fs.promises.readdir(nodeBinDirectory) without handling the case where
node_modules/.bin doesn't exist; wrap that call in a try/catch inside
prepare-local (around the nodeBinDirectory/nodeBins logic) so that on ENOENT (or
other expected filesystem errors) you set nodeBins = [] (or fallback to an empty
array) and optionally log a warning, and only call cp.execSync(sh`pnpm install`,
...) when packageBins.some(bin => !nodeBins.includes(bin)) remains true;
reference the existing symbols nodeBinDirectory, nodeBins, fs.promises.readdir,
packageBins, and cp.execSync when making the change.
1 participant
Checklist
mainhave been mergedmain