Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (16)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (8)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughSwitched repository tooling from npm/npx to pnpm: updated GitHub Actions, package manifests, docs, e2e tooling, and contributor guidance; added a pnpm workspace with catalogs; updated some tests and simplified YAML validator error handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/deploy-docs.yml (1)
33-42:⚠️ Potential issue | 🟠 MajorSame ordering issue:
corepack enableshould run beforeactions/setup-node.For consistency with the fix needed in
ci-main.yml, move thecorepack enablestep beforeactions/setup-nodeto ensure pnpm is available for cache key generation.🔧 Proposed fix
+ - run: corepack enable - name: Setup Node uses: actions/setup-node@v4 with: node-version: 20 cache: pnpm - name: Setup Pages uses: actions/configure-pages@v4 - - run: corepack enable - name: Install dependencies run: pnpm install --frozen-lockfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-docs.yml around lines 33 - 42, Move the "corepack enable" step to run before the "Setup Node" step so pnpm is available when actions/setup-node@v4 computes cache keys; specifically, reorder the workflow steps so the run: corepack enable step appears above the step named "Setup Node" (which uses actions/setup-node@v4) and ensure the "Install dependencies" step remains after setup and uses pnpm install --frozen-lockfile..github/workflows/build-docs.yml (1)
22-31:⚠️ Potential issue | 🟠 MajorSame ordering issue:
corepack enableshould run beforeactions/setup-node.For consistency with other workflows, move
corepack enablebeforeactions/setup-nodeto ensure pnpm is available for cache key generation.🔧 Proposed fix
+ - run: corepack enable - name: Setup Node uses: actions/setup-node@v4 with: node-version: 20 cache: pnpm - name: Setup Pages uses: actions/configure-pages@v4 - - run: corepack enable - name: Install dependencies run: pnpm install --frozen-lockfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-docs.yml around lines 22 - 31, Move the "corepack enable" step so it runs before the "actions/setup-node@v4" step; specifically, reorder the workflow steps so the run: corepack enable action occurs prior to the uses: actions/setup-node@v4 (and before the pnpm cache/setup) to ensure pnpm is available for cache key generation and aligns with other workflows..github/workflows/release.yml (1)
27-32:⚠️ Potential issue | 🟡 MinorMove
corepack enablebeforeactions/setup-nodeto enable pnpm caching.The
actions/setup-nodeaction withcache: pnpmrequires the pnpm binary to be available during execution so it can determine the pnpm store directory for caching. Since yourpackage.jsonspecifies"packageManager": "pnpm@10.32.1", pnpm is managed via Corepack. Runningcorepack enableaftersetup-nodemeans pnpm won't be available when the cache mechanism needs it, causing cache setup to fail.Proposed fix
+ - run: corepack enable - uses: actions/setup-node@v4 name: Setup Node.js with: node-version: 'lts/-1' cache: pnpm - - run: corepack enable🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 27 - 32, Move the "corepack enable" step to run before the actions/setup-node@v4 step so pnpm (managed by Corepack per packageManager: "pnpm@10.32.1") is available when setup-node attempts to configure pnpm caching; specifically, relocate the `run: corepack enable` step to precede the `uses: actions/setup-node@v4` step (the block with name "Setup Node.js" and with `cache: pnpm`) so the cache setup can detect the pnpm binary and store directory.
🧹 Nitpick comments (1)
packages/nx-forge/src/generators/application/generator.spec.ts (1)
10-36: Consider adding a comment explaining the mock purpose.The mocks force
addPlugin: falseto ensure explicit target configurations are generated instead of relying on Nx plugin inference. While this approach is correct for making snapshots testable, the reasoning isn't immediately apparent to future maintainers.📝 Suggested documentation
+// Force `addPlugin: false` in lint and jest generators to ensure explicit target +// configurations are created (rather than inferred by plugins), making snapshots +// deterministic and testable. jest.mock('@nx/eslint', () => { const actual = jest.requireActual<typeof import('@nx/eslint')>('@nx/eslint');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nx-forge/src/generators/application/generator.spec.ts` around lines 10 - 36, Add an inline comment above the two jest.mock blocks explaining why these mocks override `@nx/eslint.lintProjectGenerator` and `@nx/jest.configurationGenerator` to force addPlugin: false (so generated targets are explicit for deterministic snapshot testing rather than relying on Nx plugin inference); reference the mocked functions lintProjectGenerator and configurationGenerator and note that the override ensures predictable outputs in generator.spec.ts tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci-main.yml:
- Around line 19-24: Move the "corepack enable" step so pnpm is available before
the cache evaluation by actions/setup-node: ensure the workflow runs a step
executing "corepack enable" (or equivalent) before the actions/setup-node@v4
step that uses "cache: pnpm", then keep the actions/setup-node step with
"node-version: 'lts/*' cache: pnpm" and the subsequent "pnpm install
--frozen-lockfile" step unchanged; this guarantees pnpm is enabled when the
setup-node cache key is computed.
In @.github/workflows/ci-pr.yml:
- Around line 18-22: The CI step ordering is wrong: move the "corepack enable"
command to run before the actions/setup-node@v4 step so pnpm is available when
setup-node evaluates "cache: pnpm"; update the workflow so the step that runs
corepack (the run: corepack enable step) appears above the actions/setup-node@v4
step that includes with: cache: pnpm to ensure reliable pnpm caching.
In `@CONTRIBUTING.md`:
- Line 54: Fix the formatting typo in the numbered step by inserting a space
after the period: change the text "5.On the consumer side you can now install
the latest package version by running `pnpm add `@toolsplus/nx-forge`@latest`" to
"5. On the consumer side you can now install the latest package version by
running `pnpm add `@toolsplus/nx-forge`@latest`"; update the occurrence in
CONTRIBUTING.md (look for the exact string "5.On the consumer side...") so it
matches the other steps' formatting.
In `@pnpm-workspace.yaml`:
- Around line 28-31: The dependency versions for Jest are inconsistent: update
the `jest-environment-node` entry (currently `^29.4.1`) in the pnpm workspace
catalog to a 30.x release that matches the other Jest packages (e.g., `30.3.0`
or `30.0.2`) so `jest`, `jest-environment-jsdom`, `jest-util`, and
`jest-environment-node` all use compatible 30.x versions; ensure the
`jest-environment-node` value is changed to the chosen 30.x string.
---
Outside diff comments:
In @.github/workflows/build-docs.yml:
- Around line 22-31: Move the "corepack enable" step so it runs before the
"actions/setup-node@v4" step; specifically, reorder the workflow steps so the
run: corepack enable action occurs prior to the uses: actions/setup-node@v4 (and
before the pnpm cache/setup) to ensure pnpm is available for cache key
generation and aligns with other workflows.
In @.github/workflows/deploy-docs.yml:
- Around line 33-42: Move the "corepack enable" step to run before the "Setup
Node" step so pnpm is available when actions/setup-node@v4 computes cache keys;
specifically, reorder the workflow steps so the run: corepack enable step
appears above the step named "Setup Node" (which uses actions/setup-node@v4) and
ensure the "Install dependencies" step remains after setup and uses pnpm install
--frozen-lockfile.
In @.github/workflows/release.yml:
- Around line 27-32: Move the "corepack enable" step to run before the
actions/setup-node@v4 step so pnpm (managed by Corepack per packageManager:
"pnpm@10.32.1") is available when setup-node attempts to configure pnpm caching;
specifically, relocate the `run: corepack enable` step to precede the `uses:
actions/setup-node@v4` step (the block with name "Setup Node.js" and with
`cache: pnpm`) so the cache setup can detect the pnpm binary and store
directory.
---
Nitpick comments:
In `@packages/nx-forge/src/generators/application/generator.spec.ts`:
- Around line 10-36: Add an inline comment above the two jest.mock blocks
explaining why these mocks override `@nx/eslint.lintProjectGenerator` and
`@nx/jest.configurationGenerator` to force addPlugin: false (so generated targets
are explicit for deterministic snapshot testing rather than relying on Nx plugin
inference); reference the mocked functions lintProjectGenerator and
configurationGenerator and note that the override ensures predictable outputs in
generator.spec.ts tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf11fe9c-a4aa-4d01-afbe-1b9b1fe4c4f8
⛔ Files ignored due to path filters (3)
docs/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.github/workflows/build-docs.yml.github/workflows/ci-main.yml.github/workflows/ci-pr.yml.github/workflows/deploy-docs.yml.github/workflows/release.yml.gitignoreAGENTS.mdCONTRIBUTING.mddocs/package.jsone2e/nx-forge-e2e/src/utils/test-workspace.tspackage.jsonpackages/nx-forge/package.jsonpackages/nx-forge/src/generators/application/generator.spec.tspackages/nx-forge/src/utils/forge/yaml-validator.tspnpm-workspace.yamltools/docs/package.json
💤 Files with no reviewable changes (1)
- packages/nx-forge/src/utils/forge/yaml-validator.ts
tbinna
force-pushed
the
migrate-to-pnpm-catalogs
branch
3 times, most recently
from
7333c14 to
36c867b
Compare
tbinna
force-pushed
the
migrate-to-pnpm-catalogs
branch
from
36c867b to
af1e1a7
Compare
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
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.
Summary by CodeRabbit
Chores
Documentation
Tests