Skip to content

fix: install dependencies added on the other lane during lane merge #10579

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.

Sign up for GitHub

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

Open
davidfirst wants to merge 2 commits into
base: master
Choose a base branch
from
+47 −0
Open

fix: install dependencies added on the other lane during lane merge #10579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f664879
fix: install dependencies added on the other lane during lane merge
davidfirst Aug 7, 2026
6568919
fix: narrow merge cache clear to component caches, pin e2e dep version
davidfirst Aug 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions e2e/harmony/lanes/merge-lanes-main.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import chai, { expect } from 'chai';
import path from 'path';
import { Helper } from '@teambit/legacy.e2e-helper';
import chaiFs from 'chai-fs';
chai.use(chaiFs);
Expand Down Expand Up @@ -316,4 +317,43 @@ describe('merge lanes - main lane operations', function () {
expect(hashes).to.include(remoteHead);
});
});

describe('merging main into a lane when main added a new package dependency', () => {
let mergeOutput: string;
// enough distance between the top of the file (changed on main) and the bottom
// (changed on the lane) for the merge to auto-resolve without conflicts
const filler = Array.from({ length: 10 }, (unused, index) => `// filler line ${index}`).join('\n');
const baseFile = `${filler}\nmodule.exports = () => 'comp1';\n`;
before(() => {
helper.scopeHelper.setWorkspaceWithRemoteScope();
helper.fixtures.populateComponents(1);
helper.fs.outputFile('comp1/index.js', baseFile);
helper.command.tagAllWithoutBuild();
helper.command.export();
helper.command.createLane('dev');
helper.fs.outputFile('comp1/index.js', `${baseFile}// lane-only change\n`);
helper.command.snapAllComponentsWithoutBuild();
helper.command.export();
// create a lockfile that does not include is-positive
helper.command.install();
const laneWs = helper.scopeHelper.cloneWorkspace();
helper.command.switchLocalLane('main', '-x');
Comment on lines +337 to +340

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Unverified test precondition 🐞 Bug ☼ Reliability

The new e2e test claims the lane lockfile “does not include is-positive” but never asserts that
is-positive is absent before the merge, so the test can pass without actually exercising the
regression it’s meant to catch.
Agent Prompt 
### Issue description
The new e2e scenario documents that the lane workspace’s lockfile/node_modules should not contain `is-positive` before merging `main`, but it does not verify that assumption. If `is-positive` becomes present (e.g., fixture changes, transitive deps, or state leakage), the test may pass even if the merge/install behavior regresses.

### Issue Context
The setup runs `helper.command.install()` and immediately snapshots the workspace, but there is no assertion that `node_modules/is-positive` (and/or the lockfile) is actually absent at that moment.

### Fix Focus Areas
- e2e/harmony/lanes/merge-lanes-main.e2e.ts[337-347]

### Suggested fix
Add a precondition assertion right after the initial `helper.command.install()` (and before cloning/switching lanes), e.g.:
- Assert `node_modules/is-positive` does **not** exist.
- Optionally assert the lockfile does **not** mention `is-positive` (depending on the package-manager configured for these e2e runs).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

helper.command.install('is-positive@3.1.0');
helper.fs.outputFile('comp1/index.js', `const isPositive = require('is-positive');\n${baseFile}`);
Comment thread
davidfirst marked this conversation as resolved.
helper.command.tagAllWithoutBuild();
helper.command.export();
// back to the lane workspace, whose lockfile knows nothing about is-positive
helper.scopeHelper.getClonedWorkspace(laneWs);
mergeOutput = helper.command.mergeLaneWithoutBuild('main');
});
it('should install the dependency added on main and complete the auto-snap', () => {
expect(mergeOutput).to.not.have.string('snap error');
expect(mergeOutput).to.not.have.string('missing packages');
// "Total Snapped: 1" is interrupted by ANSI styling, match the section title instead
expect(mergeOutput).to.have.string('merge-snapped components (1)');
});
it('should have the new dependency in node_modules', () => {
expect(path.join(helper.scopes.localPath, 'node_modules/is-positive')).to.be.a.directory();
});
});
});
7 changes: 7 additions & 0 deletions scopes/component/merging/merging.main.runtime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,13 @@ export class MergingMain {
const leftUnresolvedConflicts = componentWithConflict && mergeStrategy === 'manual';

if (!skipDependencyInstallation && !leftUnresolvedConflicts && !componentsHasConfigMergeConflicts) {
// the components were loaded (and cached) before the merge wrote the merged files and the
// merged config (unmerged-components store). without clearing the cache, the install below
// reloads them with pre-merge dependencies, so a package newly introduced on the other side
// never enters the install manifest and the package manager skips it ("lockfile is up to date").
// clear only the component caches — the scope objects were written by this very process, so
// the scope cache is current and clearing it would add avoidable overhead to every merge.
if (this.workspace) this.workspace.clearAllComponentsCache();
// this is a workaround.
// keep this here. although it gets called before snapping.
// the reason is that when the installation is running, for some reason, some apps are unable to load in the same process.
Expand Down