Conversation
PR Summary by QodoEnable Bit global virtual store for pnpm installs
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Later installs break loaded environments
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit 9424647 |
|
Code review by qodo was updated up to the latest commit ebfb668 |
|
Code review by qodo was updated up to the latest commit 23abbf3 |
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>
|
Code review by qodo was updated up to the latest commit 82d9421 |
|
Code review by qodo was updated up to the latest commit 4ece4d2 |
|
Code review by qodo was updated up to the latest commit 3523e61 |
|
Code review by qodo was updated up to the latest commit aa13584 |
…out of reach Under the project-local layout the workspace root is an ancestor of every package's real directory, so node's walk up from any package reaches the root node_modules and a phantom import - a package requiring something it never declared - lands there. With a global virtual store a package's realpath is inside the pnpm store, the root is never an ancestor, and every such import fails. Three consumers hit this: - dependency-linker resolved @teambit/legacy and @teambit/harmony with a bare require.resolve, searching from its own realpath, and threw when it missed - taking down every bit link and bit install. It now searches the installation that holds @teambit/bit and the target workspace, and skips the link when the package is nowhere on disk: linking it is a backward-compatibility convenience for workspaces that still import it, not a reason to fail the command. - verdaccio, the e2e mock registry, died on @verdaccio/signature -> @verdaccio/config and @verdaccio/auth -> express. Both are declared through packageExtensions, pinned to versions already resolved elsewhere in the tree. - the rspack ui bundle lost @teambit/component, @teambit/docs and the node polyfills memfs and isbinaryfile import. resolve.modules now lists the host installation's node_modules chain, which also keeps a core aspect a single copy - a packageExtensions entry would have installed a second one. Also make _establishRegistry settle on every path. A verdaccio that dies without printing a port left the promise pending, and the suites run with timeout(0), so 25 of 40 e2e shards spent CircleCI's full 50-minute no-output timeout on what is now a sub-second failure carrying verdaccio's own output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnieZnVXAAyoDs5Rom9XKj
destroy runs from an `after` hook, which mocha still runs when the matching `before` failed. Without the guard a TypeError here replaces the startup error the report needs to show. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnieZnVXAAyoDs5Rom9XKj
|
Code review by qodo was updated up to the latest commit 1861a45 |
`@teambit/semantics.entities.semantic-schema` requires `@teambit/component` without declaring it - it is written against the instance the host provides. That import used to resolve on its own: the package's real directory sat inside the workspace or capsule, so the bundler's walk up from it reached a root whose node_modules holds bit's linked core aspects. Under a global virtual store the real directory is in the pnpm store, the walk leaves for the store, and three build paths fail with `Can't resolve '@teambit/component'` - the env preview template (bit_pr and custom-env-operations), the env preview strategy, and the dev server (bit start). hostDependencies is the mechanism already built for this: the alias transformer resolves each entry against [hostRootDir, cwd, __dirname] and points the bundle at the host's own copy. PHANTOM_HOST_CORE_ASPECTS is added only where those dependencies are aliased and not externalized - a component preview externalizes them, and nothing supplies a core aspect to an external at runtime. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnieZnVXAAyoDs5Rom9XKj
|
Code review by qodo was updated up to the latest commit fedb7e4 |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
…e lockfile readVirtualStoreEntries falls back to the private lockfile when no package directories are materialized, and read `packages` - the section keyed without the peer suffix. The virtual-store directory names it is comparing against come from the peer-suffixed dep paths, which is what `snapshots` is keyed by, so every peer variant of a package collapsed into one entry and `bit deps diagnose` undercounted exactly the copies it exists to report. Under the global virtual store this stopped being academic: nothing is materialized under the project's own .pnpm, so the lockfile is the only source. `packages` stays as the fallback for a lockfile written before `snapshots`. Also declare enableGlobalVirtualStore and packageExtensions in the workspace.jsonc schema, so the keys this branch adds validate and autocomplete. Both found by the Qodo review on teambit#10587. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnieZnVXAAyoDs5Rom9XKj
|
Code review by qodo was updated up to the latest commit 3618603 |
…ot supply The suite proved that a command with loaders reports a descriptive load error by creating an aspect and never installing it, then asserting on the resulting "Cannot find module '@teambit/harmony'". Under the global virtual store bit puts its own installation on NODE_PATH so that every phantom @teambit/* import resolves to the host's copy - that is what hoisted-resolution-bridge exists to do - so the aspect loads, and the suite silently stops testing anything. Give the aspect an import of a package that exists nowhere instead. The failure is then independent of the layout and of anything the host provides, and the suite keeps asserting what it was written to assert. Verified passing under both layouts: 3 passing project-local, 3 passing global-virtual-store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnieZnVXAAyoDs5Rom9XKj
|
Code review by qodo was updated up to the latest commit 4870fe4 |
…l store The suite proved that an env which cannot be loaded is reported as "(not loaded)" by emptying node_modules, so the env's @teambit/envs and @teambit/node imports would not resolve. Under the global virtual store bit puts its own installation on NODE_PATH, so those imports resolve to the host's copies by design (hoisted-resolution-bridge): the env loads, isEnvRegistered returns true, and both the marker and the NonLoadedEnv issue disappear. Give the env an import of a package that exists nowhere, on top of the empty node_modules. The env then stays unloadable for a reason no installation can repair, and the suite keeps asserting what it was written to assert. Verified passing under both layouts, and with the rest of the file: 3 passing project-local, 3 passing global-virtual-store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnieZnVXAAyoDs5Rom9XKj
|
Code review by qodo was updated up to the latest commit 507d8ab |
PreviewService builds two more targets that alias their host dependencies without externalizing them - the local-preview bundler context and the target it generates - and both were left out, so `bit start`'s local preview could still fail to resolve `@teambit/component` out of the global store. The file is .tsx, which is how it escaped the original sweep. Also state the boundary the constant is applied on: the bundle has to carry bit's own UI packages, which is where these phantom imports come from. That excludes a user's application build (react.application.ts aliases host dependencies too, but bundles user code). Found by the Qodo review on teambit#10587. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnieZnVXAAyoDs5Rom9XKj
| "resolveEnvsFromRoots": true | ||
| }, | ||
| "teambit.dependencies/dependency-resolver": { | ||
| "enableGlobalVirtualStore": true, |
There was a problem hiding this comment.
1. Later installs break loaded environments 🐞 Bug ☼ Reliability
snapshotLoadedVirtualStoreDirs() only scans <workspace>/node_modules/.pnpm, even when the installer has moved loaded package directories into the configured global store. When a subsequent install removes or re-keys one of those package slots, deferred imports from an already-loaded environment can fail because the restore phase captured nothing.
Agent Prompt
## Issue description
Enabling the global virtual store moves package slots outside the only directory inspected by the loaded-module preservation mechanism. Subsequent installs can remove or re-key a package backing an already-loaded environment, leaving deferred imports pointed at deleted files.
## Fix Focus Areas
- workspace.jsonc[14-14]
- scopes/dependencies/pnpm/pnpm.package-manager.ts[207-283]
- scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[62-130]
## Recommended Fix
Pass the effective virtual-store location into the snapshot operation and extend the snapshot/restore implementation to identify loaded package slots under both project-local and global virtual-store layouts. Preserve and restore removed global-store slots using the same loaded-module and compatible-donor safeguards currently applied to `node_modules/.pnpm`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
Accurate, and I'm deliberately not fixing it blind — leaving this thread open.
Confirmed: snapshotLoadedVirtualStoreDirs() hard-codes path.join(rootDir, 'node_modules', '.pnpm'), so under the global virtual store, where loaded slots live in <storeDir>/<version>/links, it captures nothing.
Two things narrow the impact, which is why I'd rather this were a separate change with its own reasoning than a rushed one here:
pnpmPruneModules()early-returns under this layout (readPackageDirsFromVirtualStorefinds onlylock.yamlandnode_modules), so the pruning path this mechanism guards against is inert.- The global store is content-addressed, so re-keying a package creates a new hash directory rather than removing the loaded one in place; the engine prunes the store on its own schedule.
So the failure window is narrower than in the project-local layout, but it is not closed, and the recommendation — pass the effective virtual-store location through and handle both layouts — looks right to me. Filing it as follow-up work rather than resolving it.
|
Code review by qodo was updated up to the latest commit 875913c |
The alias transformer resolves each host dependency from [hostRootDir, process.cwd(), __dirname]. This target never set the first, so the aliases - now including the phantom core aspects - were resolved from the directory the command happened to run in. That holds bit's linked core aspects, which is why preview builds pass, but it is not the env the bundle belongs to. Its sibling, ComponentBundlingStrategy, already takes the path from context.envRuntime.envAspectDefinition, and the same context is available here; the "hostRootDir, handle this" note has been open since teambit#5839. Found by the Qodo review on teambit#10587. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnieZnVXAAyoDs5Rom9XKj
Comment on lines
+21
to
+24
| "packageExtensions": { | ||
| "@verdaccio/signature": { | ||
| "dependencies": { | ||
| "@verdaccio/config": "8.0.0-next-8.1" |
There was a problem hiding this comment.
10. Fresh installs rewrite the lockfile 🐞 Bug ≡ Correctness
The new packageExtensions change pnpm's dependency graph, but the committed snapshots still omit @verdaccio/config from @verdaccio/signature and express from @verdaccio/auth. A normal `bit install` forwards these extensions to pnpm with a non-frozen lockfile, so fresh checkouts rewrite the tracked lockfile and lose reproducibility until those generated changes are committed.
Agent Prompt
## Issue description
The configured Verdaccio package extensions are absent from the committed pnpm lockfile, causing the next install to rewrite tracked dependency data.
## Fix Focus Areas
- workspace.jsonc[21-31]
- pnpm-lock.yaml[86016-86028]
- pnpm-lock.yaml[86132-86137]
## Recommended Fix
Run the repository's supported Bit/pnpm install after applying the package extensions, then commit the resulting pnpm-lock.yaml changes, including the injected dependency edges and any package-extension metadata.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+108
to
110
| output.push(data.toString()); | ||
| if (!settled && data.includes(REGISTRY_MOCK_PORT)) { | ||
| let fetchResults; |
There was a problem hiding this comment.
11. Healthy registries can fail startup 🐞 Bug ☼ Reliability
The stdout callback checks only settled, which remains false throughout the asynchronous readiness fetch and therefore allows another matching output event to start a competing fetch. If Verdaccio emits its port more than once while a probe is pending, an errored or non-200 probe can reject startup before another successful probe completes, failing every registry-backed suite.
Agent Prompt
## Issue description
Registry stdout can initiate multiple concurrent readiness requests before the startup promise settles, allowing a failed probe to win over a successful one.
## Fix Focus Areas
- components/legacy/e2e-helper/npm-ci-registry.ts[86-129]
## Recommended Fix
Add a separate readiness-probe-started flag and set it synchronously before awaiting the fetch. Keep `settled` for final promise settlement so only one probe runs while error, close, and timeout handlers remain idempotent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 533748c |
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
2 participants
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