chore: use global virtual store - #10587
Open
zkochan wants to merge 4 commits into
Open
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
PR Summary by QodoEnable Bit global virtual store for pnpm installs ⚙️ Configuration changes 🕐 10-20 Minutes
|
Sorry, something went wrong.
Copy link
Copy Markdown
Code Review by Qodo🐞 Bugs (3) 📘 Rule violations (0) 📜 Skill insights (0) Description Code Evidence Agent prompt Description Code Evidence Agent prompt 3. Persisted links path mismatch ✓ Resolved 🐞 Bug ☼ Reliability Description Code Evidence Agent prompt Description Code Evidence Agent prompt 💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more' More tips ↗ | Customize Qodo ↗ | Qodo docs ↗ Previous review resultsReview updated until commit 82d9421 ⚖️ Balanced Results up to commit 23abbf3
Description Code Evidence Agent prompt Description Code Evidence Agent prompt 3. Persisted links path mismatch ✓ Resolved 🐞 Bug ☼ Reliability Description Code Evidence Agent prompt Results up to commit ebfb668
Description Code Evidence Agent prompt Description Code Evidence Agent prompt 3. Persisted links path mismatch ✓ Resolved 🐞 Bug ☼ Reliability Description Code Evidence Agent prompt Results up to commit 9424647
Description Code Evidence Agent prompt Description Code Evidence Agent prompt Results up to commit c445ba1
Description Code Evidence Agent prompt |
Sorry, something went wrong.
Comment thread
workspace.jsonc
| "resolveEnvsFromRoots": true | ||
| }, | ||
| "teambit.dependencies/dependency-resolver": { | ||
| "enableGlobalVirtualStore": true, |
Copy link
Copy Markdown
There was a problem hiding this comment.
1. Prune deletes pnpm internals 🐞 Bug ☼ Reliability
With enableGlobalVirtualStore enabled, pnpm keeps pnpm-owned dot entries under node_modules/.pnpm, but pnpmPruneModules() does not exclude dot entries and will remove them when they don’t appear in the lockfile’s package list. This can break or destabilize subsequent installs by deleting pnpm-managed virtual-store state (or forcing it to be recreated unpredictably).Agent Prompt
### Issue description
After this PR enables `enableGlobalVirtualStore` in `workspace.jsonc`, installs run in the global virtual store layout where `node_modules/.pnpm` contains pnpm-owned entries (including dot-prefixed directories). The post-install prune step (`pnpmPruneModules`) currently treats *all* entries except `lock.yaml` and `node_modules` as prune candidates, so it may delete pnpm-owned dot entries.
### Issue Context
- The workspace runs a prune step after installs.
- Repo e2e/helper code explicitly treats dot entries under `node_modules/.pnpm` as pnpm-owned internals that should not be considered dependency directories.
### Fix Focus Areas
- scopes/dependencies/pnpm/pnpm-prune-modules.ts[21-50]
### Suggested fix
- Change `readPackageDirsFromVirtualStore()` to:
- use `readdir(..., { withFileTypes: true })`
- include **directories only**
- exclude entries that start with `.`
- keep excluding `node_modules` and `lock.yaml`
- Add/adjust an e2e or unit test for the global virtual store path to ensure prune does not remove dot-prefixed pnpm entries under `node_modules/.pnpm`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Sorry, something went wrong.
Comment thread
.circleci/config.yml
Comment on lines
+685
to
+688
| # only the global virtual store, not the content-addressable files/ it hardlinks from: | ||
| # the consumers of this workspace read node_modules, they never fetch packages, and | ||
| # carrying files/ too would duplicate every package in the archive. | ||
| - .pnpm-store/*/links |
Copy link
Copy Markdown
There was a problem hiding this comment.
1. Persisted links path mismatch 🐞 Bug ☼ Reliability
The CircleCI workspace persists .pnpm-store/*/links, but Bit resolves the global virtual store at <storeDir>/links; if the resolved storeDir is /home/circleci/bit/.pnpm-store (as configured), downstream jobs that only attach the workspace may miss the actual links directory and end up with broken node_modules symlinks.Agent Prompt
## Issue description CircleCI persists `.pnpm-store/*/links`, but Bit/pnpm’s global virtual store directory is computed as `<storeDir>/links`. If pnpm resolves `storeDir` to the configured `/home/circleci/bit/.pnpm-store`, the actual required directory would be `.pnpm-store/links`, which is not matched by `.pnpm-store/*/links`. This can break downstream jobs that rely on the attached workspace (without reinstalling) because `node_modules` entries may symlink into the missing global virtual store. ## Issue Context - CI explicitly sets `store-dir=/home/circleci/bit/.pnpm-store`. - Bit’s pnpm adapter computes the global virtual store as `join(config.storeDir, 'links')`. - Downstream jobs (e.g. `lint`) attach the workspace and run commands without reinstalling. ## Fix Focus Areas - .circleci/config.yml[665-710] ## Suggested fix Update `persist_to_workspace.paths` to persist the exact `links` directory that Bit/pnpm uses. A pragmatic, layout-tolerant option that still avoids persisting `files/` is to include both possible layouts: - `.pnpm-store/links` - `.pnpm-store/*/links` (If you want to be stricter/cleaner, ensure the persisted path exactly matches the resolved `<storeDir>/links` layout you expect in CI.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Sorry, something went wrong.
Copy link
Copy Markdown
|
Code review by qodo was updated up to the latest commit 9424647 |
Sorry, something went wrong.
Comment thread
workspace.jsonc
| "resolveEnvsFromRoots": true | ||
| }, | ||
| "teambit.dependencies/dependency-resolver": { | ||
| "enableGlobalVirtualStore": true, |
Copy link
Copy Markdown
There was a problem hiding this comment.
1. Schema missing gvs key 🐞 Bug ⚙ Maintainability
workspace.jsonc now sets enableGlobalVirtualStore, but the repo’s workspace-jsonc-schema.json does not define this property under teambit.dependencies/dependency-resolver, so schema-driven validation/autocomplete cannot surface/validate the new config key.Agent Prompt
### Issue description `workspace.jsonc` enables `enableGlobalVirtualStore`, but `workspace-jsonc-schema.json` does not declare this option in the `teambit.dependencies/dependency-resolver` schema. This creates schema/config drift: editors and any schema validation tooling won’t recognize the new key. ### Issue Context The config key is a real, supported option in code (`DependencyResolverWorkspaceConfig.enableGlobalVirtualStore?: boolean`), so the schema should be updated to match. ### Fix Focus Areas - workspace-jsonc-schema.json[51-320] - workspace.jsonc[13-16]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Sorry, something went wrong.
Copy link
Copy Markdown
|
Code review by qodo was updated up to the latest commit ebfb668 |
Sorry, something went wrong.
Copy link
Copy Markdown
|
Code review by qodo was updated up to the latest commit 23abbf3 |
Sorry, something went wrong.
With enableGlobalVirtualStore, node_modules holds only symlinks into <store-dir>/links. pnpm's default store is ~/.local/share/pnpm, outside setup_harmony's persist_to_workspace root, so every job that merely attaches the workspace received dangling symlinks (lint died on a missing node_modules/oxlint/bin/oxlint). Point store-dir under ~/bit and persist the links directory. files/ is left out: the consumers of this workspace read node_modules, they never fetch packages, and carrying the content-addressable store too would duplicate every package in the archive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
store-dir in .npmrc is ignored: pnpm keeps only npm-compatible settings there, and bit installs through @pnpm/napi's config reader, which takes storeDir from the pnpm-workspace.yaml cascade. The store stayed in ~/.local/share/pnpm, outside the persist_to_workspace root, so the node_modules symlinks reaching into <storeDir>/links still dangled in every job that only attaches the workspace. Write a CI-only pnpm-workspace.yaml instead, and check the store landed inside the workspace before persisting it — a persist path that matches nothing is not an error, so the previous attempt failed as a MODULE_NOT_FOUND in lint rather than in the job that got it wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The global virtual store puts every package's real directory outside the project, so a package that requires an undeclared dependency by bare name no longer finds it: node resolves from the realpath, and the ancestor walk out of <store>/links/@/mocha/... never reaches the project's node_modules the way the walk out of node_modules/.pnpm/mocha@11.1.0/ did. mocha requires the reporter, and mocha-multi-reporters requires each reporter it composes, so both hops died — taking down every e2e job before a single test ran. Both accept a path resolved against cwd, which is the repo root for these scripts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread
.circleci/config.yml
Comment on lines
+678
to
+679
| echo "storeDir: /home/circleci/bit/.pnpm-store" > pnpm-workspace.yaml && | ||
| echo "pnpm-workspace.yaml" >> .git/info/exclude && |
Copy link
Copy Markdown
There was a problem hiding this comment.
1. Hard-coded storedir path 🐞 Bug ⚙ Maintainability
CircleCI hard-codes the pnpm store location to /home/circleci/bit/.pnpm-store, coupling the install output to the current executor user/home + working_directory layout. If the executor image/user or working_directory ever changes, the global virtual store can end up outside the persisted workspace root and downstream jobs may get dangling node_modules symlinks.Agent Prompt
### Issue description
The CircleCI setup writes `pnpm-workspace.yaml` with an absolute `storeDir` under `/home/circleci/bit`, which is brittle if the job’s working directory or executor home ever changes.
### Issue Context
- The job working directory is configured as `~/bit`.
- `storeDir` and its verification step repeat `/home/circleci/bit` explicitly.
### Fix Focus Areas
- .circleci/config.yml[15-20]
- .circleci/config.yml[674-702]
### Suggested fix
In the `bbit install` step, compute the store path from the actual runtime directory (e.g., the outer workspace root) and use that variable consistently for:
- the `storeDir:` written into `pnpm-workspace.yaml`
- the verification step
For example, set `STORE_DIR="$(cd .. && pwd)/.pnpm-store"` (or similar, depending on where you want the store relative to the persisted root) and write `storeDir: ${STORE_DIR}`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Sorry, something went wrong.
Copy link
Copy Markdown
|
Code review by qodo was updated up to the latest commit 82d9421 |
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
At least 1 approving review is required to merge this pull request.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes