fix(nuxi): resolve upgrade lockfile across workspace and app cwd#1230
fix(nuxi): resolve upgrade lockfile across workspace and app cwd#1230
Conversation
📦 Bundle Size Comparison📈 nuxi
📈 nuxt-cli
➡️ create-nuxt
|
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1230 +/- ##
=======================================
Coverage ? 47.68%
=======================================
Files ? 47
Lines ? 1187
Branches ? 348
=======================================
Hits ? 566
Misses ? 510
Partials ? 111 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughnormaliseLockFile was changed to accept either a single directory or an array of directories (cwds) and was exported. The logic now normalizes cwds to a unique array and searches for lockfiles across all provided directories rather than a single directory. Error messaging when no lockfile is found was updated to list all searched directories. The upgrade command wiring was adjusted to pass multiple directories (workspace and project) into the new multi-directory lockfile resolution. Unit tests were added to cover the new behaviours and various input scenarios. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/nuxi/test/unit/commands/upgrade.spec.ts (2)
1-47: Good regression tests covering the core monorepo scenario.The mocking strategy is clean, and the two tests cover the primary use case (lockfile found in a fallback directory) and the error path. A few optional additional cases could strengthen coverage:
lockFilesisundefined(verifies the?.findshort-circuit)cwdspassed as a singlestring(verifies the normalization branch)- Duplicate directories in
cwds(verifiesSetdedup)- Lockfile found in the first directory (workspace root) — the most common monorepo case
These are non-blocking and can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxi/test/unit/commands/upgrade.spec.ts` around lines 1 - 47, Add the suggested extra unit tests to strengthen coverage around normaliseLockFile: add one test where lockFiles is undefined to exercise the optional chaining short-circuit, one test passing cwds as a single string to validate normalization, one test with duplicate entries in cwds to ensure deduplication (Set) works, and one test where the lockfile is found in the first directory to cover the common workspace-root case; reuse the existing vi.mock setup (existsSync and loggerError) and the same describe/beforeEach pattern so you only add new it(...) blocks that set existsSync.mockImplementation or mockReturnValue and assert the returned value and loggerError calls.
11-15: Mock fornode:fsdoesn't re-export other members.The
node:fsmock only re-exportsexistsSync. If other code imported alongsidenormaliseLockFile(or transitive imports at module evaluation time) references othernode:fsexports, this could break. Currently safe sincenormaliseLockFileonly usesexistsSync, but spreading the original module is a more resilient pattern:Suggested improvement
vi.mock('node:fs', async () => { + const actual = await vi.importActual<typeof import('node:fs')>('node:fs') return { + ...actual, existsSync, } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxi/test/unit/commands/upgrade.spec.ts` around lines 11 - 15, The test's vi.mock for 'node:fs' only returns existsSync which can break other imports; update the mock in the test so it imports the real module and spreads its exports (preserving all original members) while overriding existsSync as needed — modify the vi.mock callback used for 'node:fs' (the mock around existsSync used by normaliseLockFile) to return { ...originalFs, existsSync } instead of only { existsSync }.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/nuxi/test/unit/commands/upgrade.spec.ts`:
- Around line 1-47: Add the suggested extra unit tests to strengthen coverage
around normaliseLockFile: add one test where lockFiles is undefined to exercise
the optional chaining short-circuit, one test passing cwds as a single string to
validate normalization, one test with duplicate entries in cwds to ensure
deduplication (Set) works, and one test where the lockfile is found in the first
directory to cover the common workspace-root case; reuse the existing vi.mock
setup (existsSync and loggerError) and the same describe/beforeEach pattern so
you only add new it(...) blocks that set existsSync.mockImplementation or
mockReturnValue and assert the returned value and loggerError calls.
- Around line 11-15: The test's vi.mock for 'node:fs' only returns existsSync
which can break other imports; update the mock in the test so it imports the
real module and spreads its exports (preserving all original members) while
overriding existsSync as needed — modify the vi.mock callback used for 'node:fs'
(the mock around existsSync used by normaliseLockFile) to return {
...originalFs, existsSync } instead of only { existsSync }.
samiashi
force-pushed
the
samielachi/fix-upgrade-lockfile-detection
branch
3 times, most recently
from
81bd6d6 to
127ec70
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/nuxi/test/unit/commands/upgrade.spec.ts (4)
166-173: Consider asserting the error message content.The test verifies
loggerErrorwas called once but doesn't assert the message. A snapshot or string-match assertion on the logged message (e.g., that both/workspaceand/apps/webappear) would guard against regressions in the user-facing error output, which is part of the fix's scope.💡 Suggested assertion addition
expect(lockFile).toBeUndefined() expect(loggerError).toHaveBeenCalledTimes(1) + expect(loggerError).toHaveBeenCalledWith( + expect.stringContaining('/workspace'), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxi/test/unit/commands/upgrade.spec.ts` around lines 166 - 173, The test currently only checks that loggerError was called once when normaliseLockFile(['/workspace','/apps/web'], ['pnpm-lock.yaml']) returns undefined; update the assertion to also verify the logged error message content by inspecting loggerError.mock.calls (or using toHaveBeenCalledWith / string match) to assert it includes both '/workspace' and '/apps/web' (or take a snapshot of the message) so the user-facing error text is covered; reference the normaliseLockFile invocation and loggerError mock and keep existsSync mocked false as-is.
211-226: Integration test validates the key wiring but is fragile to internal refactors.The test asserts on specific
existsSynccall arguments ('/workspace/package-lock.json'and'/apps/web/package-lock.json'), which couples it to the internal resolution order and the exactresolve()output. If the implementation changes how paths are joined or the search order, this test breaks even if behavior is correct.A couple of observations:
The test doesn't assert the return value or outcome of
upgradeCommand.run. Adding a check (e.g., thatloggerSuccesswas called, or thataddDependencywas invoked) would confirm the command completed successfully after finding the lockfile.The
selectmock has no configured return value (defaults toundefined). This works only becausechannel: 'stable'is passed in args, presumably skipping the prompt. If the command's internal flow changes to always prompt, this test would silently pass with unexpected behavior. Consider explicitly mockingselect.mockResolvedValue('stable')to be resilient.💡 Suggested hardening
it('checks both workspace and project directories during upgrade command run', async () => { existsSync.mockImplementation((filePath: string) => filePath.endsWith('/apps/web/package-lock.json')) + select.mockResolvedValue('stable') await upgradeCommand.run!({ args: { cwd: '/apps/web', rootDir: '/apps/web', dedupe: true, force: false, channel: 'stable', }, } as any) expect(existsSync).toHaveBeenCalledWith('/workspace/package-lock.json') expect(existsSync).toHaveBeenCalledWith('/apps/web/package-lock.json') + expect(addDependency).toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxi/test/unit/commands/upgrade.spec.ts` around lines 211 - 226, The test is fragile because it asserts exact existsSync paths and doesn't verify command outcome or prompt behavior; update the test that calls upgradeCommand.run to (1) replace strict path assertions with assertions that existsSync was called for two paths that end with 'package-lock.json' (e.g., use matchers like expect.stringMatching(/package-lock\.json$/) or expect.any(String) and validate endsWith), (2) explicitly mock the prompt by adding select.mockResolvedValue('stable'), and (3) assert a visible outcome such as loggerSuccess being called or addDependency being invoked to confirm the upgradeCommand.run flow completed (referencing upgradeCommand.run, existsSync, select, loggerSuccess, addDependency).
137-155:beforeEachconfigures mocks primarily for the integration test; consider scoping them.The mocks for
detectPackageManager,findWorkspaceDir,readPackageJSON,getNuxtVersion,addDependency,dedupeDependencies,cleanupNuxtDirs,loadKit, andgetPackageManagerVersionare only exercised by the final integration test (line 211). For the purenormaliseLockFileunit tests (lines 157–209), onlyexistsSyncandloggerErrormatter. Moving these heavier mocks into a nesteddescribeor the integration test's own setup would make each test's dependencies clearer and avoid confusion if a unit test accidentally triggers an upgrade-command code path.That said, it's harmless as-is and
vi.clearAllMocks()keeps state clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxi/test/unit/commands/upgrade.spec.ts` around lines 137 - 155, The current beforeEach registers many heavy mocks (detectPackageManager, findWorkspaceDir, readPackageJSON, getNuxtVersion, addDependency, dedupeDependencies, cleanupNuxtDirs, loadKit, getPackageManagerVersion) that are only needed by the integration test exercising the upgrade path, while the unit tests for normaliseLockFile only need existsSync and loggerError; refactor by keeping a lightweight outer beforeEach that only sets up existsSync and loggerError (and vi.clearAllMocks()), and move the heavier mocks into a nested describe or the integration test's own beforeEach so tests that call normaliseLockFile only depend on minimal mocks and the integration test still configures detectPackageManager, findWorkspaceDir, readPackageJSON, getNuxtVersion, addDependency, dedupeDependencies, cleanupNuxtDirs, loadKit, and getPackageManagerVersion.
184-191: VerifyundefinedlockFiles path exercises the intended guard.When
lockFilesisundefined, the implementation doeslockFiles?.find(...)which short-circuits toundefinedwithout iterating any directories. This meansexistsSyncis never called, yet the test also setsexistsSync.mockReturnValue(false). This is fine but worth noting: the test validates the outcome (returnsundefined, logs error) but doesn't assert thatexistsSyncwas not called — which would strengthen the test by confirming the optional-chaining guard works.💡 Optional extra assertion
expect(lockFile).toBeUndefined() expect(loggerError).toHaveBeenCalledTimes(1) + expect(existsSync).not.toHaveBeenCalled()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxi/test/unit/commands/upgrade.spec.ts` around lines 184 - 191, Update the unit test to also assert that existsSync was not called to verify the optional-chaining guard; specifically, in the spec that calls normaliseLockFile(['/workspace', '/apps/web'], undefined) add an expectation that existsSync (the mocked function) hasBeenCalledTimes(0) (or toHaveBeenCalledWith not called) and keep the existing assertions on lockFile being undefined and loggerError being called once to ensure the guard short-circuits as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/nuxi/test/unit/commands/upgrade.spec.ts`:
- Around line 166-173: The test currently only checks that loggerError was
called once when normaliseLockFile(['/workspace','/apps/web'],
['pnpm-lock.yaml']) returns undefined; update the assertion to also verify the
logged error message content by inspecting loggerError.mock.calls (or using
toHaveBeenCalledWith / string match) to assert it includes both '/workspace' and
'/apps/web' (or take a snapshot of the message) so the user-facing error text is
covered; reference the normaliseLockFile invocation and loggerError mock and
keep existsSync mocked false as-is.
- Around line 211-226: The test is fragile because it asserts exact existsSync
paths and doesn't verify command outcome or prompt behavior; update the test
that calls upgradeCommand.run to (1) replace strict path assertions with
assertions that existsSync was called for two paths that end with
'package-lock.json' (e.g., use matchers like
expect.stringMatching(/package-lock\.json$/) or expect.any(String) and validate
endsWith), (2) explicitly mock the prompt by adding
select.mockResolvedValue('stable'), and (3) assert a visible outcome such as
loggerSuccess being called or addDependency being invoked to confirm the
upgradeCommand.run flow completed (referencing upgradeCommand.run, existsSync,
select, loggerSuccess, addDependency).
- Around line 137-155: The current beforeEach registers many heavy mocks
(detectPackageManager, findWorkspaceDir, readPackageJSON, getNuxtVersion,
addDependency, dedupeDependencies, cleanupNuxtDirs, loadKit,
getPackageManagerVersion) that are only needed by the integration test
exercising the upgrade path, while the unit tests for normaliseLockFile only
need existsSync and loggerError; refactor by keeping a lightweight outer
beforeEach that only sets up existsSync and loggerError (and
vi.clearAllMocks()), and move the heavier mocks into a nested describe or the
integration test's own beforeEach so tests that call normaliseLockFile only
depend on minimal mocks and the integration test still configures
detectPackageManager, findWorkspaceDir, readPackageJSON, getNuxtVersion,
addDependency, dedupeDependencies, cleanupNuxtDirs, loadKit, and
getPackageManagerVersion.
- Around line 184-191: Update the unit test to also assert that existsSync was
not called to verify the optional-chaining guard; specifically, in the spec that
calls normaliseLockFile(['/workspace', '/apps/web'], undefined) add an
expectation that existsSync (the mocked function) hasBeenCalledTimes(0) (or
toHaveBeenCalledWith not called) and keep the existing assertions on lockFile
being undefined and loggerError being called once to ensure the guard
short-circuits as intended.
samiashi
force-pushed
the
samielachi/fix-upgrade-lockfile-detection
branch
from
127ec70 to
5c00643
Compare
2 participants
🔗 Linked issue
Closes #951
❓ Type of change
📚 Description
This fixes
nuxi upgradelockfile resolution in monorepo layouts where the app directory and workspace directory differ.upgradepreviously tried to normalize lockfile candidates against a single directory (workspaceDir). In some setups this causesUnable to find any lock files...even when a valid lockfile exists for the current app context.Changes:
packages/nuxi/src/commands/upgrade.tsto search bothworkspaceDirand the commandcwd.packages/nuxi/test/unit/commands/upgrade.spec.ts:undefined+ logs error when no lockfile exists in any searched directoryValidation:
pnpm lintpnpm vitest --run packages/nuxi/test/unit/commandspnpm test:unit