migrate to pnpm #104
migrate to pnpm #104
Conversation
WalkthroughThis update transitions the project's package management and CI/CD workflows from npm and Lerna to pnpm and pnpm workspaces. It introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant pnpm
participant Node
participant Workspace Packages
Developer->>GitHub Actions: Push to main branch
GitHub Actions->>pnpm: Setup pnpm v9
GitHub Actions->>Node: Setup Node.js v20
GitHub Actions->>pnpm: Install dependencies (with cache)
pnpm->>Workspace Packages: Link and install workspace packages
GitHub Actions->>pnpm: Build packages
GitHub Actions->>pnpm: Run tests
GitHub Actions->>pnpm: Publish or export as required
Poem
✨ Finishing Touches
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 (
|
|
😭 Deploy PR Preview ead8995 failed. Build logs
🤖 By surge-preview |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
docs/adr/0002-build-system-workflow-implementation.md (1)
189-223: 🧹 Nitpick (assertive)CI workflow update is comprehensive
Switching the GitHub Actions to a unified “Changesets” job withpnpm/action-setupand caching is on-point. For even faster runs, consider pinning cache keys (e.g., lockfile checksum) and splitting install/build/test into separate cache scopes. You could also pinactions/setup-nodeandpnpm/action-setupmajor versions to avoid unexpected breaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.eslintrc.json(1 hunks).github/workflows/build.yml(2 hunks).github/workflows/deploy.yml(1 hunks).github/workflows/preview.yml(2 hunks).jest/file-mock.js(0 hunks).jest/jest.setup.after-env.js(0 hunks).jest/jest.setup.js(0 hunks).pnpm-patches/vis-timeline@7.7.0.patch(2 hunks)docs/adr/0002-build-system-workflow-implementation.md(4 hunks)global.d.ts(0 hunks)jest.config.cjs(0 hunks)lerna.json(2 hunks)package.json(2 hunks)packages/floating/copy-css.mjs(2 hunks)packages/floating/src/Tooltip/hooks/useTooltip.ts(2 hunks)packages/ui-kit/copy-css.mjs(2 hunks)packages/ui-kit/package.json(1 hunks)packages/ui-kit/src/IsochroneSlider/Slider/ColorsLegend.tsx(1 hunks)packages/ui-kit/src/IsochroneSlider/Slider/index.tsx(1 hunks)packages/ui-kit/src/utils/helpers/typecheck.ts(1 hunks)pnpm-workspace.yaml(1 hunks)
💤 Files with no reviewable changes (5)
- global.d.ts
- .jest/jest.setup.js
- .jest/file-mock.js
- .jest/jest.setup.after-env.js
- jest.config.cjs
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui-kit/copy-css.mjs
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
packages/floating/copy-css.mjs
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
🪛 actionlint (1.7.4)
.github/workflows/deploy.yml
17-17: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
21-21: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/preview.yml
16-16: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/build.yml
15-15: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
19-19: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (31)
packages/ui-kit/package.json (1)
28-28: Dependency added for default iconsThe addition of
@konturio/default-iconsdependency aligns with the pnpm migration. This provides access to standard icon sets that will be shared across the workspace.packages/ui-kit/copy-css.mjs (2)
2-2: Improved import for path moduleAdded
sepfrom path module to handle platform-specific path separators correctly.Consider updating the import to use the Node.js protocol format for better maintainability:
-import { resolve, sep } from 'path'; +import { resolve, sep } from 'node:path';🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
30-30: Enhanced cross-platform compatibilityUsing
${sep}fixture${sep}instead of a hardcoded path separator improves compatibility across different operating systems (particularly Windows where separators are backslashes).packages/floating/copy-css.mjs (2)
2-2: Improved import for path moduleAdded
sepfrom path module to handle platform-specific path separators correctly.Consider updating the import to use the Node.js protocol format for better maintainability:
-import { resolve, sep } from 'path'; +import { resolve, sep } from 'node:path';🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
30-30: Enhanced cross-platform compatibilityUsing
${sep}fixture${sep}instead of a hardcoded path separator improves compatibility across different operating systems (particularly Windows where separators are backslashes).packages/floating/src/Tooltip/hooks/useTooltip.ts (3)
12-12: Improved type importsAdded explicit import of
UseFloatingReturntype, which helps make the component's dependencies more clear.
18-23: Enhanced type safety with explicit return typeThe new
UseTooltipReturntype properly documents the shape of the hook's return value, which improves API clarity and provides better autocomplete support in consuming code.
33-33: Added explicit return type annotationApplied the newly defined type to the function signature, completing the type safety improvement.
.eslintrc.json (1)
59-60: Disable base rule before enabling TS-specific rule
Disablingno-unused-expressionsbefore enabling@typescript-eslint/no-unused-expressionsprevents duplicate lint errors and aligns with TypeScript-ESLint best practices.pnpm-workspace.yaml (1)
1-2: Workspace inclusion looks correct
Specifyingpackages: ['packages/*']correctly includes all workspace packages underpackages/. Ensure it matches your monorepo layout..pnpm-patches/vis-timeline@7.7.0.patch (2)
9-10: Extend cluster rendering with contentTemplate
The added conditional foroptions.contentTemplateallows users to override the default cluster markup cleanly while preserving the original behavior when no template is provided.
18-21: AddcontentTemplateto options schema
IncludingcontentTemplatein theallOptions$1schema ensures the new option is recognized by the library’s internal type mappings, maintaining consistency with the patched logic above.lerna.json (2)
5-5: Switch Lerna client to pnpm
Updating"npmClient": "pnpm"aligns Lerna with the new package manager across the monorepo.
14-14: Update bootstrap arguments for pnpm
Replacing npm flags with["--no-lockfile"]prevents lockfile creation during bootstrap and matches pnpm’s conventions..github/workflows/deploy.yml (4)
13-15: 🚀 pnpm setup is correct
The newpnpm/action-setup@v2step withversion: 9properly initializes pnpm for the workspace migration.
16-19: ✅ Node.js version bump is valid
Switching to Node.js 20 viaactions/setup-node@v3aligns with our updated runtime requirements.🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
20-27: ⚡️ Caching pnpm store speeds up CI
The cache configuration for~/.pnpm-storekeyed onpnpm-lock.yamlis set up correctly to reduce install times.🧰 Tools
🪛 actionlint (1.7.4)
21-21: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
30-33: 🔄 Migrate build commands to pnpm
Replacingnpm ci/npm runwithpnpm install --frozen-lockfile,pnpm build, andpnpm cosmos:build:exportaligns with the pnpm migration.packages/ui-kit/src/IsochroneSlider/Slider/index.tsx (1)
15-15: ✅ DefaultclassNameprop and PropTypes removal
Providing an empty string default forclassNameensures the component never receivesundefined. Removing PropTypes in favor of TypeScript typings keeps the codebase consistent..github/workflows/preview.yml (3)
12-15: 🚀 pnpm setup is correct
Usingpnpm/action-setup@v2withversion: 9ensures pnpm is correctly installed for the preview job.
16-21: ⚡️ pnpm cache configuration
Caching~/.pnpm-storekeyed by the lockfile is configured properly to speed up dependency installs on PR previews.🧰 Tools
🪛 actionlint (1.7.4)
16-16: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
30-33: 🔄 Build and export via pnpm
Switching the surge-preview build block topnpm install --frozen-lockfile,pnpm build, andpnpm cosmos:build:exportaligns with the new package manager.packages/ui-kit/src/IsochroneSlider/Slider/ColorsLegend.tsx (1)
8-8: ✅ DefaultclassNameprop and PropTypes removal
SettingclassName = ''by default and removing PropTypes matches the TypeScript-first approach used elsewhere in the UI Kit..github/workflows/build.yml (5)
11-14: 🚀 pnpm setup is correct
Addingpnpm/action-setup@v2withversion: 9is consistent and prepares pnpm for the build pipeline.
15-17: ✅ Node.js version bump is valid
Usingactions/setup-node@v3to lock to Node.js 20 matches our updated runtime targets.🧰 Tools
🪛 actionlint (1.7.4)
15-15: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
19-24: ⚡️ pnpm cache configuration
Configuring cache for~/.pnpm-storewith the lockfile hash is correctly set up to optimize CI performance.🧰 Tools
🪛 actionlint (1.7.4)
19-19: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
28-33: 🔄 Convert install/bootstrap/test to pnpm
Commands for installing, globally installing Lerna, bootstrapping, linting, building, and testing have been correctly migrated to pnpm equivalents.
46-46: 🔄 Update publish step to pnpm
Switchingnpm run auto:lerna:publishtopnpm auto:lerna:publishcompletes the end-to-end migration.package.json (2)
63-63: Verify lint-staged integration
The lint-staged config triggerspnpm lint:fixfor TS files. Ensure that lint-staged runs within a PNPM context (e.g., viapnpm exec) so that thelint:fixscript is resolvable.
111-114: Patch configuration for vis-timeline@7.7.0
Thepnpm.patchedDependenciesentry correctly applies your local patch. Ensure the.pnpm-patches/vis-timeline@7.7.0.patchfile is present and documents the upstream issue you’re fixing.docs/adr/0002-build-system-workflow-implementation.md (1)
178-180: Ensure scripts alignment with implementation
Theci:publishandreleasescripts here should match what’s in your rootpackage.json. Double-check that you invokepnpm changeset versionconsistently (CLI name vs. script name) and that anpnpm installprecedes publishing in CI.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
package.json(2 hunks)
🔇 Additional comments (2)
package.json (2)
12-16: Scripts migration is well implemented
The core build, watch, dev, bootstrap, and cosmos-export scripts have been correctly updated to use pnpm commands and recursive workflows.
102-106: Patch configuration correctly applied for pnpm
Yourpnpm.patchedDependenciesfield is properly configured forvis-timeline@7.7.0, ensuring the workaround patch is applied during installation.
…linting and versioning
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
package.json (2)
67-67: 🛠️ Refactor suggestionMove React to peerDependencies
As a UI library,reactandreact-domshould be declared underpeerDependenciesto avoid bundling duplicate React instances in consuming applications.
83-83: 🧹 Nitpick (assertive)Remove unused Lerna dependency
Thelernapackage remains indevDependenciesbut is no longer used after migrating to pnpm. You can safely remove it to reduce installation size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(2 hunks)package.json(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yml
15-15: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
19-19: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (9)
.github/workflows/build.yml (5)
11-13: pnpm setup action configured correctly
Thepnpm/action-setup@v2step pins pnpm to version 9 as intended.
14-17: Node.js environment setup is appropriate
Usingactions/setup-node@v3to install Node.js 20 matches the project’s runtime requirements.🧰 Tools
🪛 actionlint (1.7.4)
15-15: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
18-24: Caching the pnpm store will improve CI performance
The cache configuration for~/.pnpm-storewith a lockfile-based key is correct and should reduce install times.🧰 Tools
🪛 actionlint (1.7.4)
19-19: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-31: Replace npm steps with pnpm workspace commands
Switching fromnpm ci/Lerna topnpm install --frozen-lockfileandpnpm bootstrap, followed by the pnpm-based lint and build scripts, aligns perfectly with the migration goals.
44-44: Publish step correctly updated to pnpm
Thepnpm publish:packagescommand replaces the previous Lerna publish step and leverages the new workspace publishing script.package.json (4)
12-17: Migration of core scripts to pnpm workspaces looks solid
Thebuild,build:watch,dev,bootstrap,cosmos:build:export, and rootpublishscripts have been updated to use pnpm commands and workspace filters. This aligns with the monorepo migration and maintains intended behaviors.
21-24: Versioning and cleanup scripts now use pnpm
Thereleaseandversion:no-pushscripts correctly leveragepnpm version, andts:cleanensures build artifacts are removed across all workspaces. Great use of pnpm’s native tooling.
26-26: Auto-lint script fixed to referencelint:fix
Updatingauto:lintto runpnpm -r lint:fixaddresses the undefinedlinttask from previous iterations.
101-105: Verify pnpm patch file inclusion
A newpnpmfield patchesvis-timeline@7.7.0. Ensure that the file.pnpm-patches/vis-timeline@7.7.0.patchis committed and correctly applied during installation.
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
setup-references.mjs (1)
76-83: 🧹 Nitpick (assertive)Parameter name shadows imported
path
const pathExists = (path) => { ... }shadows the earlierimport path from 'path';, which can confuse readers and tools.-const pathExists = (path) => { +const pathExists = (p) => { try { - accessSync(path, constants.R_OK); + accessSync(p, constants.R_OK);
♻️ Duplicate comments (2)
package.json (2)
56-56: 🧹 Nitpick (assertive)Optimize lint-staged for TypeScript files
Running the fullpnpm lint:fixin a lint-staged hook is overkill. Consider targeting only staged files:- "*/**/*.{ts,tsx}": ["pnpm lint:fix"] + "*/**/*.{ts,tsx}": ["eslint --fix", "git add"]This scopes fixes to just the staged files and speeds up the hook.
66-67: 🛠️ Refactor suggestionDeclare React as a peer dependency
To avoid bundling multiple React instances in downstream apps, move"react"and"react-dom"fromdependenciesinto apeerDependenciessection:- "dependencies": { - "react": "^18.2.0", - "react-dom": "^18.2.0", + "peerDependencies": { + "react": "^18.2.0", + "react-dom": "^18.2.0" + }, + "dependencies": { … }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.eslintrc.json(2 hunks).github/workflows/build.yml(2 hunks).github/workflows/deploy.yml(1 hunks)package.json(2 hunks)setup-references.mjs(4 hunks)tsconfig.json(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/deploy.yml
17-17: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/build.yml
15-15: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 GitHub Actions: Lint, Build, Test, Publish
tsconfig.json
[error] 1-1: TypeScript error TS5083: Cannot read file '/home/runner/work/ui/ui/packages/tsconfig.json'.
🪛 Biome (1.9.4)
setup-references.mjs
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
🔇 Additional comments (5)
.eslintrc.json (2)
81-84: Globs may break editor-side linting – use explicit paths
"packages/*/tsconfig.json"works in CLI mode, but several IDE plug-ins (e.g. VS Code ESLint) do not expand globs inside theprojectoption, which leads to “Cannot read file …” errors while editing.
Consider enumerating workspaces or pointing to a generated umbrella config (e.g.packages/tsconfig.json) instead:- "project": [ - "tsconfig.json", - "packages/*/tsconfig.json" - ] + "project": ["tsconfig.json", "packages/tsconfig.json"]This also aligns with the
setup-references.mjsscript that writespackages/tsconfig.json.
59-60: 🧹 Nitpick (assertive)Prefer relying only on the TS-aware rule
Turning off the base
no-unused-expressionsand re-enabling the TypeScript-aware variant is the right call, but you can simplify maintenance by omitting the first line entirely: eslint automatically disables the base rule when the@typescript-eslintoverride is present.
This keeps the config shorter and avoids potential rule-order confusion.- "no-unused-expressions": "off",Likely an incorrect or invalid review comment.
.github/workflows/build.yml (1)
22-27:pnpm bootstrapis not a built-in commandUnless a custom
bootstrapscript exists inpackage.json, this step will fail.
Common patterns:# 1) use recursive install pnpm install --recursive --frozen-lockfile # or 2) run your custom lifecycle script pnpm run bootstrapPlease verify that the command resolves.
package.json (2)
21-23: Scripts correctly updated to use pnpm versioning
Thereleaseandversion:no-pushscripts now leveragepnpm -r version, aligning with pnpm’s native versioning workflow.
102-105: Verify the vis-timeline patch file
Please confirm that./.pnpm-patches/vis-timeline@7.7.0.patchis committed in the repo and that pnpm will correctly apply it during install.
| pnpm install --frozen-lockfile | ||
| pnpm build | ||
| pnpm cosmos:build:export |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fail fast on install – add --filter ... or CI flags if possible
Running a full pnpm install in a monorepo can be slow. Consider:
pnpm install --frozen-lockfile --filter ./packages/... # only workspace packages
pnpm install --frozen-lockfile --prefer-offline # network-resilient
Not mandatory, but it can shave minutes off CI time for large workspaces.
🤖 Prompt for AI Agents (early access)
In .github/workflows/deploy.yml around lines 26 to 28, the pnpm install command runs a full install which can be slow in a monorepo. To speed up CI, modify the install command to include either a --filter flag targeting only workspace packages or add --prefer-offline to reduce network calls. For example, use 'pnpm install --frozen-lockfile --filter ./packages/...' or 'pnpm install --frozen-lockfile --prefer-offline' to optimize install time.
| - uses: pnpm/action-setup@v2 | ||
| with: | ||
| version: 9 | ||
| - name: Setup pnpm cache | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: ~/.pnpm-store | ||
| key: ${{ runner.os }}-pnpm-${{ hashFiles('**/pnpm-lock.yaml') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-pnpm- |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leverage pnpm/action-setup built-in cache
pnpm/action-setup@v2 already supports caching via with: { cache: true }.
The extra actions/cache@v3 step is redundant and occasionally causes the “runner too old” warning flagged by action-lint.
You can drop the manual step and enable the built-in cache:
- - uses: pnpm/action-setup@v2
- with:
- version: 9
- - name: Setup pnpm cache
- uses: actions/cache@v3
- with:
- path: ~/.pnpm-store
- key: ${{ runner.os }}-pnpm-${{ hashFiles('**/pnpm-lock.yaml') }}
- restore-keys: |
- ${{ runner.os }}-pnpm-
+ - uses: pnpm/action-setup@v2
+ with:
+ version: 9
+ cache: trueThis lowers maintenance and avoids path mismatches (~/.pnpm-store is no longer the default in pnpm ≥ 7).
📝 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.
| - uses: pnpm/action-setup@v2 | |
| with: | |
| version: 9 | |
| - name: Setup pnpm cache | |
| uses: actions/cache@v3 | |
| with: | |
| path: ~/.pnpm-store | |
| key: ${{ runner.os }}-pnpm-${{ hashFiles('**/pnpm-lock.yaml') }} | |
| restore-keys: | | |
| ${{ runner.os }}-pnpm- | |
| - uses: pnpm/action-setup@v2 | |
| with: | |
| version: 9 | |
| cache: true |
🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents (early access)
In .github/workflows/deploy.yml around lines 13 to 22, the workflow redundantly uses actions/cache@v3 to cache pnpm store, which is unnecessary because pnpm/action-setup@v2 supports built-in caching. Remove the entire actions/cache@v3 step and instead add 'cache: true' under the 'with' section of pnpm/action-setup@v2 to enable its built-in cache feature, simplifying the workflow and avoiding path mismatches.
| - uses: pnpm/action-setup@v2 | ||
| with: | ||
| version: 9 | ||
| - name: Setup pnpm cache | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: ~/.pnpm-store | ||
| key: ${{ runner.os }}-pnpm-${{ hashFiles('**/pnpm-lock.yaml') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-pnpm- | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same caching remark as in deploy workflow
actions/cache@v3 can be removed and replaced with cache: true in pnpm/action-setup.
See the diff suggestion in the deploy workflow comment.
🧰 Tools
🪛 actionlint (1.7.4)
15-15: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents (early access)
In .github/workflows/build.yml around lines 11 to 21, replace the usage of actions/cache@v3 for pnpm caching by removing the entire cache setup step and instead add cache: true under the pnpm/action-setup@v2 step. This simplifies the workflow by leveraging the built-in caching feature of pnpm/action-setup.
| echo "registry=https://registry.npmjs.org/" >> .npmrc | ||
| echo "//registry.npmjs.org/:_authToken=${{ secrets.NPM_AUTH_TOKEN }}" >> .npmrc | ||
| npm run auto:lerna:publish | ||
| pnpm publish:packages |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make publishing idempotent
pnpm publish fails if the version already exists. Consider --no-git-checks --access public --filter … or using changeset to guard against duplicate publishes.
🤖 Prompt for AI Agents (early access)
In .github/workflows/build.yml at line 40, the current `pnpm publish:packages` command can fail if the package version already exists, causing the publish step to break. Modify the publish command to include flags like `--no-git-checks --access public --filter ...` to make the publishing process idempotent, or integrate a tool like `changeset` to prevent duplicate publishes by checking for changes before publishing.
| "paths": { | ||
| "@konturio/*": [ | ||
| "packages/*/src" | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Path mapping could hide external scopes
"@konturio/*": ["packages/*/src"] matches both workspace packages and any third-party module whose name starts with @konturio/.
If you ever consume a scoped external dependency, it will be shadowed.
Consider narrowing the glob:
- "@konturio/*": ["packages/*/src"]
+ "@konturio/*": ["packages/*/src/index.ts"]or introduce a secondary scope (e.g. @konturio-internal/).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In tsconfig.json around lines 42 to 46, the path mapping "@konturio/*": ["packages/*/src"] is too broad and may shadow external scoped packages starting with "@konturio/". To fix this, narrow the glob pattern to only match your internal packages more specifically or change the scope prefix to something like "@konturio-internal/*" to avoid conflicts with external dependencies.
| const packages = topology.map((pkg) => pkg.replace(CONFIG.npmNamespace, CONFIG.packagesFolder)); | ||
| console.log('Packages paths:', packages); | ||
|
|
||
| const tsPackages = takeOnlyTypescriptPackages(packages); | ||
| const tsConfig = createTsConfigWithReferences(tsPackages); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Namespace-to-path replacement is brittle
pkg.replace('@konturio', './packages') silently breaks if the package name contains the namespace twice or in a different position.
Safer approach:
const toPath = (name) =>
path.join(CONFIG.packagesFolder, name.replace(`${CONFIG.npmNamespace}/`, ''));
...
const packages = topology.map(toPath);Prevents accidental partial replacements.
🤖 Prompt for AI Agents (early access)
In setup-references.mjs around lines 113 to 117, the current namespace-to-path replacement uses a simple string replace that can incorrectly modify package names containing the namespace multiple times or in unexpected positions. To fix this, define a function that removes the namespace prefix only if it appears at the start of the package name, then joins the result with the packages folder path using path.join. Replace the existing map call with this function to ensure safer and more accurate path conversion.
| @@ -1,4 +1,4 @@ | |||
| import { constants, accessSync } from 'fs'; | |||
| import { constants, accessSync, existsSync, mkdirSync } from 'fs'; | |||
| import { writeFile } from 'fs/promises'; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use the node: import protocol
Biome flagged this: built-in modules should be imported with the node: prefix for clarity and ESM compatibility.
-import { constants, accessSync, existsSync, mkdirSync } from 'fs';
+import { constants, accessSync, existsSync, mkdirSync } from 'node:fs';Repeat for fs/promises, util, path, and child_process.
📝 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.
| import { constants, accessSync, existsSync, mkdirSync } from 'fs'; | |
| -import { constants, accessSync, existsSync, mkdirSync } from 'fs'; | |
| +import { constants, accessSync, existsSync, mkdirSync } from 'node:fs'; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
🤖 Prompt for AI Agents (early access)
In setup-references.mjs at line 1, update all imports from built-in Node.js modules to use the `node:` prefix for clarity and ESM compatibility. Change imports like 'fs', 'fs/promises', 'util', 'path', and 'child_process' to 'node:fs', 'node:fs/promises', 'node:util', 'node:path', and 'node:child_process' respectively.
| "open-cli": "^7.0.0", | ||
| "patch-package": "6.4.7", | ||
| "postcss-cli": "^10.0.0", | ||
| "lerna": "^4.0.0", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused Lerna dependency
Since all Lerna commands have been migrated to pnpm, the "lerna": "^4.0.0" entry in devDependencies is no longer needed and can be removed to slim down installs.
🤖 Prompt for AI Agents (early access)
In package.json at line 83, remove the "lerna": "^4.0.0" entry from the devDependencies section because Lerna is no longer used and all commands have been migrated to pnpm, so this dependency is unnecessary and should be deleted to reduce install size.
| "build": "pnpm ts:clean && pnpm auto:ts:references && pnpm auto:ts:build && pnpm -r build", | ||
| "build:watch": "pnpm auto:ts:references && pnpm auto:ts:build:watch && pnpm -r build", | ||
| "dev": "pnpm auto:cosmos", | ||
| "bootstrap": "pnpm -r install", | ||
| "cosmos:build:export": "cosmos-export --config cosmos.config.export.json", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Leverage pnpm filters to simplify workspace builds
You can replace the broad pnpm -r build (and watch) calls with pnpm’s built-in filtering, for example:
pnpm -r --filter "ui/**" build
pnpm -r --filter "ui/**" build:watchThis lets you scope commands to your workspace packages and could eliminate the need for custom auto:ts:* orchestration scripts.
🤖 Prompt for AI Agents (early access)
In package.json lines 12 to 16, the current build and build:watch scripts use broad recursive calls with pnpm -r build, which can be simplified. Replace these with pnpm recursive commands that use the --filter option to target specific workspace packages, such as pnpm -r --filter "ui/**" build and pnpm -r --filter "ui/**" build:watch. This change scopes the build commands to relevant packages and may allow removal of the custom auto:ts:* scripts.
| "ts:clean": "pnpm -r exec -- rimraf tslib && tsc --build --clean", | ||
| "ts:watch": "tsc --build --watch", | ||
| "auto:lerna:publish": "lerna publish from-package --yes --no-verify-access", | ||
| "auto:lerna:version": "lerna version", | ||
| "auto:lint": "lerna run lint && eslint \"*/**/*.{ts,tsx}\"", | ||
| "auto:lint": "pnpm lint:fix", |
There was a problem hiding this comment.
Make auto-lint run across all workspaces
Currently auto:lint only invokes the top-level lint:fix. To lint every workspace, switch to the recursive form:
- "auto:lint": "pnpm lint:fix",
+ "auto:lint": "pnpm -r lint:fix",📝 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.
| "ts:clean": "pnpm -r exec -- rimraf tslib && tsc --build --clean", | |
| "ts:watch": "tsc --build --watch", | |
| "auto:lerna:publish": "lerna publish from-package --yes --no-verify-access", | |
| "auto:lerna:version": "lerna version", | |
| "auto:lint": "lerna run lint && eslint \"*/**/*.{ts,tsx}\"", | |
| "auto:lint": "pnpm lint:fix", | |
| "ts:clean": "pnpm -r exec -- rimraf tslib && tsc --build --clean", | |
| "ts:watch": "tsc --build --watch", | |
| "auto:lint": "pnpm -r lint:fix", |
🤖 Prompt for AI Agents (early access)
In package.json lines 24 to 26, the "auto:lint" script only runs the top-level "lint:fix" and does not cover all workspaces. Update the "auto:lint" script to run "pnpm -r lint:fix" to recursively execute linting across all workspaces, ensuring every package is linted.
Summary by CodeRabbit
Chores
Bug Fixes
Refactor
useTooltiphook.Documentation