Merged
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:
📝 WalkthroughWalkthroughRemoved the deprecated Changes
sequenceDiagram
autonumber
participant Dev as Developer (CLI)
participant Nx as Nx (nx run)
participant PackageExec as Package Executor
participant FS as Filesystem (dist)
participant ForgeCLI as Forge CLI
Dev->>Nx: nx run app:package
Nx->>PackageExec: invoke package executor
PackageExec->>PackageExec: patch manifest & process resources
PackageExec->>FS: write packaged app to dist/apps/app
PackageExec-->>Nx: return success
Dev->>ForgeCLI: (cwd=dist/apps/app) forge install --non-interactive ...
ForgeCLI->>FS: read packaged files & manifest
ForgeCLI-->>Dev: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nx-forge/src/generators/application/generator.spec.ts (1)
159-164:⚠️ Potential issue | 🟠 MajorTemplate
tsconfig.app.jsonmust be updated to includejest.config.ctsin theexcludearray.Tests at lines 159–164 and 352–357 expect the
excludearray to contain:["jest.config.ts", "src/**/*.spec.ts", "src/**/*.test.ts", "jest.config.cts"]The template at
packages/nx-forge/src/generators/application/files/tsconfig.app.jsoncurrently excludes onlyjest.config.ts(missingjest.config.cts), causing test failures. Update line 12 to include thejest.config.ctsentry.🤖 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 159 - 164, The tsconfig.app.json template's exclude array is missing "jest.config.cts", causing tsconfigApp.exclude assertions to fail; update the generator template tsconfig.app.json to add "jest.config.cts" to the exclude array so that tsconfigApp.exclude equals ["jest.config.ts", "src/**/*.spec.ts", "src/**/*.test.ts", "jest.config.cts"] and the failing tests pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/guides/getting-started.md`:
- Around line 236-240: Replace the invalid Nx invocation "nx run
<nx-forge-app-name>:forge install --site <my-atlassian-site.atlassian.net>
--product jira --non-interactive" with the correct Forge executor syntax used
elsewhere: "nx forge <nx-forge-app-name> install --site
<my-atlassian-site.atlassian.net> --product jira --non-interactive" so the
command uses the "nx forge <app-name> install ..." form rather than attempting
to run a nested target.
In `@package.json`:
- Around line 46-49: The dependency versions are inconsistent:
"jest-environment-node" is pinned to ^29.4.1 while other Jest packages ("jest",
"jest-environment-jsdom", "jest-util") are at 30.0.5; update the
"jest-environment-node" entry in package.json to the matching 30.0.5 version (or
use a consistent semver range) so all Jest-related packages use the same major
version to avoid runtime incompatibilities.
---
Outside diff comments:
In `@packages/nx-forge/src/generators/application/generator.spec.ts`:
- Around line 159-164: The tsconfig.app.json template's exclude array is missing
"jest.config.cts", causing tsconfigApp.exclude assertions to fail; update the
generator template tsconfig.app.json to add "jest.config.cts" to the exclude
array so that tsconfigApp.exclude equals ["jest.config.ts", "src/**/*.spec.ts",
"src/**/*.test.ts", "jest.config.cts"] and the failing tests pass.
🪄 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: bb3ba8f1-5e28-47b0-b14a-5c6db7c10911
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
.gitignoreAGENTS.mddocs/concepts/plugin-concepts.mddocs/guides/adding-a-ui-kit-module.mddocs/guides/generating-a-forge-app.mddocs/guides/getting-started.mddocs/guides/migrating-to-inferred-tasks.mddocs/guides/migrating-to-package-executor.mddocs/reference/executors.mde2e/nx-forge-e2e/tests/basic-setup.spec.tse2e/nx-forge-e2e/tests/build.executor.spec.tsnx.jsonpackage.jsonpackages/nx-forge/executors.jsonpackages/nx-forge/package.jsonpackages/nx-forge/project.jsonpackages/nx-forge/src/executors/build/executor.tspackages/nx-forge/src/executors/build/lib/compile-webpack.tspackages/nx-forge/src/executors/build/lib/generate-package-json.tspackages/nx-forge/src/executors/build/lib/normalize-options.tspackages/nx-forge/src/executors/build/schema.d.tspackages/nx-forge/src/executors/build/schema.jsonpackages/nx-forge/src/executors/install/executor.tspackages/nx-forge/src/executors/install/schema.d.tspackages/nx-forge/src/executors/install/schema.jsonpackages/nx-forge/src/executors/package/executor.tspackages/nx-forge/src/executors/package/lib/copy-forge-app-assets.tspackages/nx-forge/src/executors/package/lib/patch-manifest-yml.tspackages/nx-forge/src/executors/package/lib/process-resource-dependencies.tspackages/nx-forge/src/generators/application/files/README.mdpackages/nx-forge/src/generators/application/files/manifest.ymlpackages/nx-forge/src/generators/application/generator.spec.tspackages/nx-forge/src/graph/create-nodes.tspackages/nx-forge/src/migrations/update-3-0-0-webpack-config-setup/update-3-0-0-webpack-config-setup.tspackages/nx-forge/src/shared/targets/get-install-config.tstools/docs/src/lib/reference-docs.spec.tstools/scripts/start-local-registry.ts
💤 Files with no reviewable changes (14)
- packages/nx-forge/src/executors/package/lib/copy-forge-app-assets.ts
- packages/nx-forge/src/executors/install/schema.d.ts
- packages/nx-forge/src/graph/create-nodes.ts
- packages/nx-forge/src/executors/build/schema.json
- e2e/nx-forge-e2e/tests/build.executor.spec.ts
- packages/nx-forge/src/executors/build/lib/normalize-options.ts
- packages/nx-forge/executors.json
- packages/nx-forge/src/shared/targets/get-install-config.ts
- packages/nx-forge/src/executors/install/executor.ts
- packages/nx-forge/src/executors/build/schema.d.ts
- packages/nx-forge/src/executors/build/executor.ts
- packages/nx-forge/src/executors/build/lib/compile-webpack.ts
- packages/nx-forge/src/executors/install/schema.json
- packages/nx-forge/src/executors/build/lib/generate-package-json.ts
Comment on lines
+46
to
+49
| "jest": "30.0.5", | ||
| "jest-environment-jsdom": "30.0.5", | ||
| "jest-environment-node": "^29.4.1", | ||
| "nx": "20.3.1", | ||
| "jest-util": "30.0.5", |
There was a problem hiding this comment.
Version mismatch: jest-environment-node is pinned to 29.x while other Jest packages are at 30.x.
Line 48 has jest-environment-node at ^29.4.1, which is inconsistent with jest (30.0.5), jest-environment-jsdom (30.0.5), and jest-util (30.0.5). This version mismatch could cause runtime issues or unexpected behavior.
🐛 Proposed fix
"jest": "30.0.5",
"jest-environment-jsdom": "30.0.5",
- "jest-environment-node": "^29.4.1",
+ "jest-environment-node": "30.0.5",
"jest-util": "30.0.5",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "jest": "30.0.5", | |
| "jest-environment-jsdom": "30.0.5", | |
| "jest-environment-node": "^29.4.1", | |
| "nx": "20.3.1", | |
| "jest-util": "30.0.5", | |
| "jest": "30.0.5", | |
| "jest-environment-jsdom": "30.0.5", | |
| "jest-environment-node": "30.0.5", | |
| "jest-util": "30.0.5", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 46 - 49, The dependency versions are inconsistent:
"jest-environment-node" is pinned to ^29.4.1 while other Jest packages ("jest",
"jest-environment-jsdom", "jest-util") are at 30.0.5; update the
"jest-environment-node" entry in package.json to the matching 30.0.5 version (or
use a consistent semver range) so all Jest-related packages use the same major
version to avoid runtime incompatibilities.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/nx-forge/src/generators/application/generator.spec.ts (4)
351-357:⚠️ Potential issue | 🟠 MajorSame template mismatch issue for nested project tests.
This test also expects
'jest.config.cts'in the exclude array, but the template doesn't include it. This assertion will fail for the same reason as the non-nested test.🤖 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 351 - 357, The test's expectedValue array includes 'jest.config.cts' which the template doesn't produce; update the nested-project test by removing 'jest.config.cts' from the expectedValue array (the case with lookupFn: (json) => json.exclude) so the assertion matches the template's actual exclude entries.
360-363:⚠️ Potential issue | 🔴 CriticalSame flat ESLint config issue for nested projects.
This assertion checks for
.eslintrc.jsonwhich no longer exists in Nx 22. The pipeline failure at line 334 confirms this. Update to check for the flat ESLint config file instead.🤖 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 360 - 363, The test is still asserting the legacy nested ESLint file path 'my-dir/my-forge-app/.eslintrc.json' and expectedValue accordingly; update the assertion to check the flat ESLint config used by Nx 22 instead by replacing the path string with the flat file name (e.g., 'my-dir/my-forge-app/.eslintrc') and update the lookup/expectedValue (the lookupFn and expectedValue array) to reference the flattened extends target (e.g., '../../.eslintrc') so the assertion in generator.spec.ts matches the new flat ESLint config format.
159-164:⚠️ Potential issue | 🟠 MajorTest assertion includes
jest.config.ctsthat the template doesn't generate.The test expects
tsconfigApp.excludeto contain'jest.config.cts', but the template file atpackages/nx-forge/src/generators/application/files/tsconfig.app.jsononly includes"jest.config.ts"in the exclude array. This mismatch will cause the test to fail.Either add
"jest.config.cts"to the template's exclude array, or remove it from the test assertion.🤖 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 159 - 164, The test's expected exclude array contains 'jest.config.cts' which the generated tsconfig template doesn't include; update the test assertion that checks tsconfigApp.exclude to remove 'jest.config.cts' so it matches the tsconfig.app.json template's exclude (which only contains 'jest.config.ts'), or alternatively add 'jest.config.cts' to the template's exclude array—adjust the code that builds the expectation around tsconfigApp.exclude accordingly.
166-201:⚠️ Potential issue | 🔴 CriticalUpdate test to verify flat ESLint config format instead of legacy
.eslintrc.json.Nx 22's
lintProjectGeneratorcreates flat config files (eslint.config.js/mjs/cjs) by default, not the legacy.eslintrc.jsonformat. The test at lines 166-201 attempts to readmy-forge-app/.eslintrc.json, which the generator no longer creates. This will causereadJsonto fail since the file does not exist.Update the test to verify the generated flat config file and its expected structure instead:
- Change
readJson(tree, 'my-forge-app/.eslintrc.json')to read the appropriateeslint.config.*file generated by the linter- Update the inline snapshot to match the flat config format (array of config objects with
files,rules, etc.) rather than the legacy.eslintrc.jsonstructure (extends,overrides, etc.)🤖 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 166 - 201, The test is reading the now-nonexistent legacy file 'my-forge-app/.eslintrc.json' via readJson; change the test in generator.spec.ts to read the generated flat ESLint config (e.g., the project's eslint.config.* output) instead of .eslintrc.json and update the inline snapshot to the flat config shape (an array of config objects with "files" and "rules" entries and any top-level plugins/parsers as produced by lintProjectGenerator). Locate the readJson call and the expect(...).toMatchInlineSnapshot invocation and replace the file path and snapshot content to assert the flat eslint.config structure.
♻️ Duplicate comments (1)
package.json (1)
47-50:⚠️ Potential issue | 🟠 MajorAlign
jest-environment-nodewith the Jest 30 upgrade.
jest-environment-nodeis still on 29.x while the rest of the Jest runtime stack here has moved to 30.0.5. Jest generally recommends keeping Jest packages on the same version, and this environment is selected at runtime viatestEnvironment: 'node'. (archive.jestjs.io)🐛 Proposed fix
"jest": "30.0.5", "jest-environment-jsdom": "30.0.5", - "jest-environment-node": "^29.4.1", + "jest-environment-node": "30.0.5", "jest-util": "30.0.5",#!/bin/bash set -euo pipefail echo "Jest package versions:" rg -n '"jest":|"jest-environment-node":|"jest-environment-jsdom":|"jest-util":' package.json echo echo "Configured Jest environment:" rg -n 'testEnvironment' jest.preset.js packages/nx-forge/jest.config.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 47 - 50, The jest-environment-node dependency in package.json is pinned to 29.x while other Jest packages are 30.0.5; update the "jest-environment-node" entry to "30.0.5" to align versions so testEnvironment: 'node' uses the same Jest 30 runtime, and run your package manager install to update lockfile and verify tests; look for the "jest-environment-node" key in package.json to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/.vitepress/config.mts`:
- Around line 10-15: The config uses resolve(__dirname, ...) which will throw in
this ES module; replace __dirname usage by deriving the directory from
import.meta.url via fileURLToPath (fileURLToPath(import.meta.url)) and
path.dirname, then pass that directory into resolve when building the path for
jsonataGrammar; update the jsonataGrammar initialization (and any other
resolve(...) calls) to use the computed dirname so resolve(...) uses a valid
filesystem path in this ESM config.
- Around line 5-8: The package.json for the tools/docs package is pointing its
"main" field at a non-existent ./src/index.js; update tools/docs/package.json's
"main" field so imports used in docs/.vitepress/config.mts (e.g.,
injectReferenceOptions and validateReferenceDocs) resolve correctly — either set
"main" to the actual source entry "./src/index.ts" if you want to import the TS
source directly, or change it to the built output (e.g., the compiled dist path
such as "./dist/tools/docs/index.js") depending on whether consumers should use
the built package.
In `@docs/guides/adding-a-ui-kit-module.md`:
- Line 11: The sentence currently uses "UI Kit compatible packaging" without a
hyphen; update the copy to read "UI Kit-compatible packaging" for grammatical
consistency and clarity wherever it references the `uiKit2Packaging` option (the
`package` target documentation), replacing the unhyphenated phrase with the
hyphenated form.
---
Outside diff comments:
In `@packages/nx-forge/src/generators/application/generator.spec.ts`:
- Around line 351-357: The test's expectedValue array includes 'jest.config.cts'
which the template doesn't produce; update the nested-project test by removing
'jest.config.cts' from the expectedValue array (the case with lookupFn: (json)
=> json.exclude) so the assertion matches the template's actual exclude entries.
- Around line 360-363: The test is still asserting the legacy nested ESLint file
path 'my-dir/my-forge-app/.eslintrc.json' and expectedValue accordingly; update
the assertion to check the flat ESLint config used by Nx 22 instead by replacing
the path string with the flat file name (e.g., 'my-dir/my-forge-app/.eslintrc')
and update the lookup/expectedValue (the lookupFn and expectedValue array) to
reference the flattened extends target (e.g., '../../.eslintrc') so the
assertion in generator.spec.ts matches the new flat ESLint config format.
- Around line 159-164: The test's expected exclude array contains
'jest.config.cts' which the generated tsconfig template doesn't include; update
the test assertion that checks tsconfigApp.exclude to remove 'jest.config.cts'
so it matches the tsconfig.app.json template's exclude (which only contains
'jest.config.ts'), or alternatively add 'jest.config.cts' to the template's
exclude array—adjust the code that builds the expectation around
tsconfigApp.exclude accordingly.
- Around line 166-201: The test is reading the now-nonexistent legacy file
'my-forge-app/.eslintrc.json' via readJson; change the test in generator.spec.ts
to read the generated flat ESLint config (e.g., the project's eslint.config.*
output) instead of .eslintrc.json and update the inline snapshot to the flat
config shape (an array of config objects with "files" and "rules" entries and
any top-level plugins/parsers as produced by lintProjectGenerator). Locate the
readJson call and the expect(...).toMatchInlineSnapshot invocation and replace
the file path and snapshot content to assert the flat eslint.config structure.
---
Duplicate comments:
In `@package.json`:
- Around line 47-50: The jest-environment-node dependency in package.json is
pinned to 29.x while other Jest packages are 30.0.5; update the
"jest-environment-node" entry to "30.0.5" to align versions so testEnvironment:
'node' uses the same Jest 30 runtime, and run your package manager install to
update lockfile and verify tests; look for the "jest-environment-node" key in
package.json to make the change.
🪄 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: d13de725-fcc1-49a4-983e-95952a73ab67
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
.eslintignore.eslintrc.json.gitignoreAGENTS.mddocs/.vitepress/config.mtsdocs/.vitepress/grammars/jsonata.tmLanguage.jsondocs/concepts/plugin-concepts.mddocs/guides/adding-a-ui-kit-module.mddocs/guides/generating-a-forge-app.mddocs/guides/getting-started.mddocs/guides/migrating-to-inferred-tasks.mddocs/guides/migrating-to-package-executor.mddocs/guides/transforming-the-manifest.mddocs/reference/executors.mde2e/nx-forge-e2e/tests/basic-setup.spec.tse2e/nx-forge-e2e/tests/build.executor.spec.tseslint.config.mjsnx.jsonpackage.jsonpackages/nx-forge/.eslintrc.jsonpackages/nx-forge/eslint.config.mjspackages/nx-forge/executors.jsonpackages/nx-forge/jest.config.tspackages/nx-forge/package.jsonpackages/nx-forge/project.jsonpackages/nx-forge/src/executors/build/executor.tspackages/nx-forge/src/executors/build/lib/compile-webpack.tspackages/nx-forge/src/executors/build/lib/generate-package-json.tspackages/nx-forge/src/executors/build/lib/normalize-options.tspackages/nx-forge/src/executors/build/schema.d.tspackages/nx-forge/src/executors/build/schema.jsonpackages/nx-forge/src/executors/install/executor.tspackages/nx-forge/src/executors/install/schema.d.tspackages/nx-forge/src/executors/install/schema.jsonpackages/nx-forge/src/executors/package/executor.tspackages/nx-forge/src/executors/package/lib/copy-forge-app-assets.tspackages/nx-forge/src/executors/package/lib/create-package-json.tspackages/nx-forge/src/executors/package/lib/patch-manifest-yml.tspackages/nx-forge/src/executors/package/lib/process-resource-dependencies.tspackages/nx-forge/src/generators/application/files/README.mdpackages/nx-forge/src/generators/application/files/manifest.ymlpackages/nx-forge/src/generators/application/generator.spec.tspackages/nx-forge/src/graph/create-nodes.tspackages/nx-forge/src/migrations/update-2-3-0/add-nx-forge-plugin.tspackages/nx-forge/src/migrations/update-2-3-0/remove-implicit-custom-ui-dependencies.tspackages/nx-forge/src/migrations/update-3-0-0-webpack-config-setup/update-3-0-0-webpack-config-setup.tspackages/nx-forge/src/shared/targets/get-install-config.tstools/docs/.eslintrc.jsontools/docs/eslint.config.mjstools/docs/src/lib/reference-docs.spec.tstools/scripts/start-local-registry.ts
💤 Files with no reviewable changes (22)
- .eslintignore
- packages/nx-forge/src/migrations/update-2-3-0/add-nx-forge-plugin.ts
- packages/nx-forge/jest.config.ts
- packages/nx-forge/src/executors/package/lib/copy-forge-app-assets.ts
- packages/nx-forge/src/graph/create-nodes.ts
- packages/nx-forge/src/executors/package/lib/create-package-json.ts
- packages/nx-forge/src/migrations/update-2-3-0/remove-implicit-custom-ui-dependencies.ts
- packages/nx-forge/src/executors/install/schema.d.ts
- tools/docs/.eslintrc.json
- packages/nx-forge/.eslintrc.json
- packages/nx-forge/executors.json
- packages/nx-forge/src/shared/targets/get-install-config.ts
- packages/nx-forge/src/executors/build/schema.json
- packages/nx-forge/src/executors/build/lib/compile-webpack.ts
- packages/nx-forge/src/executors/install/executor.ts
- e2e/nx-forge-e2e/tests/build.executor.spec.ts
- .eslintrc.json
- packages/nx-forge/src/executors/build/lib/normalize-options.ts
- packages/nx-forge/src/executors/build/schema.d.ts
- packages/nx-forge/src/executors/build/executor.ts
- packages/nx-forge/src/executors/install/schema.json
- packages/nx-forge/src/executors/build/lib/generate-package-json.ts
✅ Files skipped from review due to trivial changes (10)
- AGENTS.md
- packages/nx-forge/package.json
- docs/guides/migrating-to-package-executor.md
- docs/guides/transforming-the-manifest.md
- .gitignore
- packages/nx-forge/src/executors/package/lib/process-resource-dependencies.ts
- packages/nx-forge/project.json
- docs/guides/migrating-to-inferred-tasks.md
- docs/guides/generating-a-forge-app.md
- tools/docs/src/lib/reference-docs.spec.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/nx-forge/src/generators/application/files/manifest.yml
- packages/nx-forge/src/executors/package/executor.ts
- tools/scripts/start-local-registry.ts
- docs/concepts/plugin-concepts.md
- packages/nx-forge/src/generators/application/files/README.md
- docs/guides/getting-started.md
- packages/nx-forge/src/migrations/update-3-0-0-webpack-config-setup/update-3-0-0-webpack-config-setup.ts
- e2e/nx-forge-e2e/tests/basic-setup.spec.ts
- docs/reference/executors.md
- nx.json
- packages/nx-forge/src/executors/package/lib/patch-manifest-yml.ts
Comment on lines
5
to
+8
| import { | ||
| injectReferenceOptions, | ||
| validateReferenceDocs, | ||
| } from '../../tools/docs/src/lib/reference-docs'; | ||
| } from '../../tools/docs'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "docs config import:"
sed -n '1,12p' docs/.vitepress/config.mts
echo
echo "tools/docs package entry:"
sed -n '1,40p' tools/docs/package.json
echo
echo "available source entry files:"
find tools/docs/src -maxdepth 1 \( -name 'index.ts' -o -name 'index.js' \) -print | sortRepository: toolsplus/nx-forge
Length of output: 723
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== All files in tools/docs/src ==="
ls -la tools/docs/src/ 2>/dev/null || echo "Directory listing failed"
echo
echo "=== Check for compiled index.js anywhere in tools/docs ==="
find tools/docs -name 'index.js' -type f 2>/dev/null || echo "No index.js found"
echo
echo "=== Check build/package scripts ==="
grep -A 5 '"scripts"' tools/docs/package.json || echo "No scripts section"
echo
echo "=== Check if there's a build config (tsconfig, esbuild, etc) ==="
ls -la tools/docs/ | grep -E '\.(tsconfig|esbuild|rollup|webpack)' || echo "No obvious build configs at root"
echo
echo "=== Check tsconfig in tools directory ==="
if [ -f tools/tsconfig.json ]; then
echo "Found tools/tsconfig.json - first 30 lines:"
head -30 tools/tsconfig.json
fi
echo
echo "=== Check if tools/docs has its own tsconfig ==="
if [ -f tools/docs/tsconfig.json ]; then
echo "Found tools/docs/tsconfig.json:"
cat tools/docs/tsconfig.json
else
echo "No tsconfig.json in tools/docs"
fiRepository: toolsplus/nx-forge
Length of output: 905
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Content of tools/docs/src/index.ts ==="
cat tools/docs/src/index.ts
echo
echo "=== Root vite/build config ==="
ls -la | grep -E '(vite|build)' || echo "No vite/build files at root"
echo
echo "=== Check if docs has its own vite config ==="
find docs -name 'vite.config.*' -o -name 'vitest.config.*' 2>/dev/null | head -5
echo
echo "=== Check package.json to see if it defines exports field (modern resolution) ==="
grep -E '"(exports|browser|module)"' tools/docs/package.json || echo "No exports/browser/module fields"
echo
echo "=== Check nx.json or project.json for build setup ==="
if [ -f tools/docs/project.json ]; then
echo "Found tools/docs/project.json - showing first 50 lines:"
head -50 tools/docs/project.json
else
echo "No project.json in tools/docs"
fiRepository: toolsplus/nx-forge
Length of output: 1743
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check docs/project.json for build configuration ==="
if [ -f docs/project.json ]; then
cat docs/project.json
else
echo "No docs/project.json found"
fi
echo
echo "=== Check root tsconfig for path mapping ==="
if [ -f tsconfig.base.json ]; then
cat tsconfig.base.json | head -100
fi
echo
echo "=== Check if docs has dependency on tools-docs build ==="
grep -r 'tools.docs\|tools/docs' nx.json 2>/dev/null | head -20 || echo "No references found"Repository: toolsplus/nx-forge
Length of output: 853
🏁 Script executed:
#!/bin/bash
set -euo pipefamerikansk
echo "=== Check VitePress version and config ==="
grep -i 'vitepress' docs/.vitepress/config.mts package.json pnpm-lock.yaml 2>/dev/null | head -20 || echo "Checking package files..."
if [ -f package.json ]; then
echo "VitePress in package.json:"
grep 'vitepress' package.json || echo "Not found in package.json"
fi
echo
echo "=== Check if there's a build script or dev script that handles this ==="
grep -A 5 '"scripts"' package.json | head -20
echo
echo "=== Verify what the relative import actually resolves to ==="
echo "Import path: ../../tools/docs from docs/.vitepress/config.mts"
echo "This resolves to: tools/docs (directory)"
echo "Node would then check: tools/docs/package.json#main = ./src/index.js"
echo "Which would resolve to: tools/docs/src/index.js"
echo
echo "File existence check:"
[ -f tools/docs/src/index.js ] && echo "✓ index.js EXISTS" || echo "✗ index.js DOES NOT EXIST"
[ -f tools/docs/src/index.ts ] && echo "✓ index.ts EXISTS" || echo "✗ index.ts DOES NOT EXIST"Repository: toolsplus/nx-forge
Length of output: 122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check VitePress version and config ==="
if [ -f package.json ]; then
echo "VitePress in package.json:"
grep 'vitepress' package.json || echo "Not found in package.json"
fi
echo
echo "=== Check if there's a build script or dev script that handles this ==="
grep -A 5 '"scripts"' package.json | head -20
echo
echo "=== Verify what the relative import actually resolves to ==="
echo "Import path: ../../tools/docs from docs/.vitepress/config.mts"
echo "This resolves to: tools/docs (directory)"
echo "Node would then check: tools/docs/package.json#main = ./src/index.js"
echo "Which would resolve to: tools/docs/src/index.js"
echo
echo "File existence check:"
[ -f tools/docs/src/index.js ] && echo "✓ index.js EXISTS" || echo "✗ index.js DOES NOT EXIST"
[ -f tools/docs/src/index.ts ] && echo "✓ index.ts EXISTS" || echo "✗ index.ts DOES NOT EXIST"Repository: toolsplus/nx-forge
Length of output: 722
Update tools/docs/package.json to correctly reference the source entry point.
The import in docs/.vitepress/config.mts resolves to tools/docs as a directory. Node module resolution then checks tools/docs/package.json#main, which points to ./src/index.js. However, this file does not exist—only tools/docs/src/index.ts is checked in, and the Nx build outputs to dist/tools/docs. Change the "main" field to "./src/index.ts" to match the actual source entry point, or update it to point to the compiled output path if the package should be consumed post-build.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/.vitepress/config.mts` around lines 5 - 8, The package.json for the
tools/docs package is pointing its "main" field at a non-existent
./src/index.js; update tools/docs/package.json's "main" field so imports used in
docs/.vitepress/config.mts (e.g., injectReferenceOptions and
validateReferenceDocs) resolve correctly — either set "main" to the actual
source entry "./src/index.ts" if you want to import the TS source directly, or
change it to the built output (e.g., the compiled dist path such as
"./dist/tools/docs/index.js") depending on whether consumers should use the
built package.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e/nx-forge-e2e/tests/basic-setup.spec.ts (1)
124-124: Optional: extract packaged app cwd into a constant to avoid path drift.This path is repeated for install/uninstall and could be centralized.
♻️ Proposed refactor
+ const packagedAppCwd = joinPathFragments( + tmpProjPath(), + 'dist', + 'apps', + appName + ); + const installResult = await runForgeCommandAsync( `install --product=${installationContext.product} --site=${installationContext.siteUrl} --environment ${installationContext.environment} --non-interactive`, { - cwd: joinPathFragments(tmpProjPath(), 'dist', 'apps', appName), + cwd: packagedAppCwd, silenceError: true, } ); @@ const uninstallResult = await runForgeCommandAsync( `uninstall --product=${installationContext.product} --site=${installationContext.siteUrl} --environment ${installationContext.environment}`, { - cwd: joinPathFragments(tmpProjPath(), 'dist', 'apps', appName), + cwd: packagedAppCwd, silenceError: true, } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nx-forge-e2e/tests/basic-setup.spec.ts` at line 124, Extract the repeated packaged-app working directory into a single constant to avoid path drift: compute the path once using joinPathFragments(tmpProjPath(), 'dist', 'apps', appName) and assign it to a clearly named constant (e.g., packagedAppCwd) at the top of the test or describe block, then replace the inline occurrences (the cwd arguments used in install/uninstall calls) with that constant; reference functions/identifiers joinPathFragments, tmpProjPath, and appName to locate the places to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nx-forge/eslint.config.mjs`:
- Around line 1-7: The workspace lint cache setup only watches
{workspaceRoot}/eslint.config.mjs, so edits to this package's local
eslint.config.mjs (packages/nx-forge/eslint.config.mjs) can produce stale lint
cache; update the nx.json cache invalidation for the `@nx/eslint`:lint target to
also include this package file path (packages/nx-forge/eslint.config.mjs) so
changing the package-level eslint.config.mjs will rerun ESLint.
In `@packages/nx-forge/src/generators/application/generator.spec.ts`:
- Around line 366-370: The test in generator.spec.ts that checks for absent Jest
config files should also assert the workspace-root renamed file is not emitted;
add an expectation alongside the other checks to verify
tree.exists('jest.config.cts') is falsy (e.g.,
expect(tree.exists('jest.config.cts')).toBeFalsy();) inside the same test block
that contains the existing assertions for 'jest.config.ts' and
jestConfigPath('my-forge-app').
---
Nitpick comments:
In `@e2e/nx-forge-e2e/tests/basic-setup.spec.ts`:
- Line 124: Extract the repeated packaged-app working directory into a single
constant to avoid path drift: compute the path once using
joinPathFragments(tmpProjPath(), 'dist', 'apps', appName) and assign it to a
clearly named constant (e.g., packagedAppCwd) at the top of the test or describe
block, then replace the inline occurrences (the cwd arguments used in
install/uninstall calls) with that constant; reference functions/identifiers
joinPathFragments, tmpProjPath, and appName to locate the places to change.
🪄 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: 47374452-db96-423b-8faf-2391131ec813
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (52)
.eslintignore.eslintrc.json.gitignoreAGENTS.mddocs/.vitepress/config.mtsdocs/.vitepress/grammars/jsonata.tmLanguage.jsondocs/concepts/plugin-concepts.mddocs/guides/adding-a-ui-kit-module.mddocs/guides/generating-a-forge-app.mddocs/guides/getting-started.mddocs/guides/migrating-to-inferred-tasks.mddocs/guides/migrating-to-package-executor.mddocs/guides/transforming-the-manifest.mddocs/reference/executors.mde2e/nx-forge-e2e/tests/basic-setup.spec.tse2e/nx-forge-e2e/tests/build.executor.spec.tseslint.config.mjsnx.jsonpackage.jsonpackages/nx-forge/.eslintrc.jsonpackages/nx-forge/eslint.config.mjspackages/nx-forge/executors.jsonpackages/nx-forge/jest.config.tspackages/nx-forge/package.jsonpackages/nx-forge/project.jsonpackages/nx-forge/src/executors/build/executor.tspackages/nx-forge/src/executors/build/lib/compile-webpack.tspackages/nx-forge/src/executors/build/lib/generate-package-json.tspackages/nx-forge/src/executors/build/lib/normalize-options.tspackages/nx-forge/src/executors/build/schema.d.tspackages/nx-forge/src/executors/build/schema.jsonpackages/nx-forge/src/executors/install/executor.tspackages/nx-forge/src/executors/install/schema.d.tspackages/nx-forge/src/executors/install/schema.jsonpackages/nx-forge/src/executors/package/executor.tspackages/nx-forge/src/executors/package/lib/copy-forge-app-assets.tspackages/nx-forge/src/executors/package/lib/create-package-json.tspackages/nx-forge/src/executors/package/lib/patch-manifest-yml.tspackages/nx-forge/src/executors/package/lib/process-resource-dependencies.tspackages/nx-forge/src/generators/application/files/README.mdpackages/nx-forge/src/generators/application/files/manifest.ymlpackages/nx-forge/src/generators/application/files/tsconfig.app.jsonpackages/nx-forge/src/generators/application/generator.spec.tspackages/nx-forge/src/graph/create-nodes.tspackages/nx-forge/src/migrations/update-2-3-0/add-nx-forge-plugin.tspackages/nx-forge/src/migrations/update-2-3-0/remove-implicit-custom-ui-dependencies.tspackages/nx-forge/src/migrations/update-3-0-0-webpack-config-setup/update-3-0-0-webpack-config-setup.tspackages/nx-forge/src/shared/targets/get-install-config.tstools/docs/.eslintrc.jsontools/docs/eslint.config.mjstools/docs/src/lib/reference-docs.spec.tstools/scripts/start-local-registry.ts
💤 Files with no reviewable changes (22)
- packages/nx-forge/src/executors/package/lib/copy-forge-app-assets.ts
- packages/nx-forge/jest.config.ts
- packages/nx-forge/src/migrations/update-2-3-0/add-nx-forge-plugin.ts
- packages/nx-forge/src/migrations/update-2-3-0/remove-implicit-custom-ui-dependencies.ts
- .eslintignore
- tools/docs/.eslintrc.json
- packages/nx-forge/src/executors/package/lib/create-package-json.ts
- packages/nx-forge/executors.json
- packages/nx-forge/src/shared/targets/get-install-config.ts
- packages/nx-forge/src/executors/build/schema.json
- packages/nx-forge/src/executors/build/lib/normalize-options.ts
- packages/nx-forge/src/executors/build/executor.ts
- packages/nx-forge/src/executors/build/lib/compile-webpack.ts
- packages/nx-forge/src/executors/install/executor.ts
- packages/nx-forge/src/executors/build/schema.d.ts
- e2e/nx-forge-e2e/tests/build.executor.spec.ts
- packages/nx-forge/.eslintrc.json
- packages/nx-forge/src/executors/install/schema.d.ts
- packages/nx-forge/src/executors/build/lib/generate-package-json.ts
- .eslintrc.json
- packages/nx-forge/src/executors/install/schema.json
- packages/nx-forge/src/graph/create-nodes.ts
✅ Files skipped from review due to trivial changes (16)
- .gitignore
- packages/nx-forge/package.json
- packages/nx-forge/src/generators/application/files/manifest.yml
- docs/concepts/plugin-concepts.md
- docs/guides/migrating-to-package-executor.md
- AGENTS.md
- docs/guides/transforming-the-manifest.md
- packages/nx-forge/src/executors/package/executor.ts
- docs/guides/generating-a-forge-app.md
- packages/nx-forge/src/migrations/update-3-0-0-webpack-config-setup/update-3-0-0-webpack-config-setup.ts
- packages/nx-forge/src/generators/application/files/tsconfig.app.json
- packages/nx-forge/project.json
- docs/guides/getting-started.md
- tools/docs/src/lib/reference-docs.spec.ts
- docs/reference/executors.md
- docs/.vitepress/grammars/jsonata.tmLanguage.json
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/guides/adding-a-ui-kit-module.md
- packages/nx-forge/src/executors/package/lib/process-resource-dependencies.ts
- docs/guides/migrating-to-inferred-tasks.md
- tools/docs/eslint.config.mjs
- tools/scripts/start-local-registry.ts
- nx.json
- docs/.vitepress/config.mts
- packages/nx-forge/src/generators/application/files/README.md
- packages/nx-forge/src/executors/package/lib/patch-manifest-yml.ts
Comment on lines
+1
to
+7
| import baseConfig from '../../eslint.config.mjs'; | ||
|
|
||
| export default [ | ||
| { | ||
| ignores: ['**/dist', '**/out-tsc'], | ||
| }, | ||
| ...baseConfig, |
There was a problem hiding this comment.
Track project-level flat configs in the lint cache.
nx.json:11-19 only invalidates @nx/eslint:lint on {workspaceRoot}/eslint.config.mjs. Now that this project has its own eslint.config.mjs, changing it can return a stale cached lint result instead of rerunning ESLint.
Suggested follow-up in nx.json
"@nx/eslint:lint": {
"inputs": [
"default",
"{workspaceRoot}/.eslintrc.json",
- "{workspaceRoot}/eslint.config.mjs"
+ "{workspaceRoot}/eslint.config.mjs",
+ "{projectRoot}/eslint.config.mjs"
],
"cache": true
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nx-forge/eslint.config.mjs` around lines 1 - 7, The workspace lint
cache setup only watches {workspaceRoot}/eslint.config.mjs, so edits to this
package's local eslint.config.mjs (packages/nx-forge/eslint.config.mjs) can
produce stale lint cache; update the nx.json cache invalidation for the
`@nx/eslint`:lint target to also include this package file path
(packages/nx-forge/eslint.config.mjs) so changing the package-level
eslint.config.mjs will rerun ESLint.
BREAKING CHANGE: requires Nx 22, removed build and install executors Closes #181
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/nx-forge-e2e/tests/utils/async-commands.ts (1)
108-114:⚠️ Potential issue | 🟡 MinorAdd early return after
reject(same issue asrunCommandAsync).Both
rejectandresolvecan be called whensilenceErroris false and an error occurs.🛠️ Proposed fix
(err, stdout, stderr) => { if (!opts.silenceError && err) { console.error('Failed to run Forge command:', err, stdout, stderr); - reject(err); + return reject(err); } resolve({ stdout, stderr }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nx-forge-e2e/tests/utils/async-commands.ts` around lines 108 - 114, The callback handling in the async command runner can call both reject and resolve when opts.silenceError is false and err exists; update the anonymous callback (the one receiving (err, stdout, stderr) used by runForgeCommandAsync / runCommandAsync) to return immediately after calling reject(err) (e.g., change "reject(err);" to "return reject(err);") or wrap resolve in an else branch so that once an error triggers reject the function does not continue to call resolve({ stdout, stderr }). Ensure you modify the callback that checks "if (!opts.silenceError && err)" and the equivalent callback in runCommandAsync.
🧹 Nitpick comments (2)
e2e/nx-forge-e2e/tests/utils/async-commands.ts (1)
100-100: Inconsistent package manager detection compared torunNxCommandAsync.
runNxCommandAsyncusesdetectPackageManager(cwd)to detect the package manager for the target directory, whilerunForgeCommandAsynccallsgetPackageManagerCommand()without arguments (uses current working directory detection). If forge commands are run in a cwd that uses a different package manager, this could produce incorrect exec commands.♻️ Proposed fix for consistency
export const runForgeCommandAsync = ( command: string, opts: { silenceError?: boolean; env?: NodeJS.ProcessEnv; cwd?: string } = { silenceError: false, } ): Promise<{ stdout: string; stderr: string }> => { - const pmc = getPackageManagerCommand(); + const cwd = opts.cwd ?? tmpProjPath(); + const pmc = getPackageManagerCommand(detectPackageManager(cwd)); return new Promise((resolve, reject) => { exec( `${pmc.exec} forge ${command}`, { - cwd: opts.cwd ?? tmpProjPath(), + cwd, env: getCommandEnv(opts.env), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nx-forge-e2e/tests/utils/async-commands.ts` at line 100, runForgeCommandAsync is using getPackageManagerCommand() (cwd-agnostic) which can mismatch runNxCommandAsync's detectPackageManager(cwd); update the package manager lookup in runForgeCommandAsync to call detectPackageManager(cwd) (instead of getPackageManagerCommand()) and use that returned command for the exec; ensure you import/retain detectPackageManager and replace references to getPackageManagerCommand so pmc is derived from the target cwd.package.json (1)
39-40: Remove unused standalone TypeScript ESLint packages from Lines 39-40.Under flat-config with
typescript-eslint(line 59), the standalone@typescript-eslint/eslint-pluginand@typescript-eslint/parserpackages are redundant. No config files or code in the repository import them directly. Remove these lines to clean up dependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 39 - 40, Remove the redundant standalone TypeScript ESLint packages by deleting the "@typescript-eslint/eslint-plugin" and "@typescript-eslint/parser" entries from package.json (these are redundant because the project uses the flat-config "typescript-eslint" entry); ensure no other files import those exact package names and then run dependency install to update lockfile (e.g., npm/yarn/pnpm install) to reflect the removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/nx-forge-e2e/tests/utils/async-commands.ts`:
- Around line 47-52: The callback passed to the async command helper currently
calls reject(err) then continues to call resolve({ stdout, stderr }) which is
confusing; update the handler (the callback that receives (err, stdout, stderr)
in e2e/nx-forge-e2e/tests/utils/async-commands.ts) to return immediately after
calling reject (or use an else) when err is truthy and opts.silenceError is
false so resolve is not invoked thereafter; locate the block referencing
opts.silenceError, err, reject and resolve and add a `return` (or convert to `if
(...) { reject(err); return; }`) to stop execution.
---
Outside diff comments:
In `@e2e/nx-forge-e2e/tests/utils/async-commands.ts`:
- Around line 108-114: The callback handling in the async command runner can
call both reject and resolve when opts.silenceError is false and err exists;
update the anonymous callback (the one receiving (err, stdout, stderr) used by
runForgeCommandAsync / runCommandAsync) to return immediately after calling
reject(err) (e.g., change "reject(err);" to "return reject(err);") or wrap
resolve in an else branch so that once an error triggers reject the function
does not continue to call resolve({ stdout, stderr }). Ensure you modify the
callback that checks "if (!opts.silenceError && err)" and the equivalent
callback in runCommandAsync.
---
Nitpick comments:
In `@e2e/nx-forge-e2e/tests/utils/async-commands.ts`:
- Line 100: runForgeCommandAsync is using getPackageManagerCommand()
(cwd-agnostic) which can mismatch runNxCommandAsync's detectPackageManager(cwd);
update the package manager lookup in runForgeCommandAsync to call
detectPackageManager(cwd) (instead of getPackageManagerCommand()) and use that
returned command for the exec; ensure you import/retain detectPackageManager and
replace references to getPackageManagerCommand so pmc is derived from the target
cwd.
In `@package.json`:
- Around line 39-40: Remove the redundant standalone TypeScript ESLint packages
by deleting the "@typescript-eslint/eslint-plugin" and
"@typescript-eslint/parser" entries from package.json (these are redundant
because the project uses the flat-config "typescript-eslint" entry); ensure no
other files import those exact package names and then run dependency install to
update lockfile (e.g., npm/yarn/pnpm install) to reflect the removal.
🪄 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: a1ae0fe6-b608-4ee8-867a-f9e32cf5630d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (56)
.codex/config.toml.eslintignore.eslintrc.json.gitignoreAGENTS.mddocs/.vitepress/config.mtsdocs/.vitepress/grammars/jsonata.tmLanguage.jsondocs/concepts/plugin-concepts.mddocs/guides/adding-a-ui-kit-module.mddocs/guides/generating-a-forge-app.mddocs/guides/getting-started.mddocs/guides/migrating-to-inferred-tasks.mddocs/guides/migrating-to-package-executor.mddocs/guides/transforming-the-manifest.mddocs/reference/executors.mde2e/nx-forge-e2e/tests/application.generator.spec.tse2e/nx-forge-e2e/tests/basic-setup.spec.tse2e/nx-forge-e2e/tests/build.executor.spec.tse2e/nx-forge-e2e/tests/utils/async-commands.tse2e/nx-forge-e2e/tests/utils/generate-forge-app.tseslint.config.mjsnx.jsonpackage.jsonpackages/nx-forge/.eslintrc.jsonpackages/nx-forge/eslint.config.mjspackages/nx-forge/executors.jsonpackages/nx-forge/jest.config.tspackages/nx-forge/package.jsonpackages/nx-forge/project.jsonpackages/nx-forge/src/executors/build/executor.tspackages/nx-forge/src/executors/build/lib/compile-webpack.tspackages/nx-forge/src/executors/build/lib/generate-package-json.tspackages/nx-forge/src/executors/build/lib/normalize-options.tspackages/nx-forge/src/executors/build/schema.d.tspackages/nx-forge/src/executors/build/schema.jsonpackages/nx-forge/src/executors/install/executor.tspackages/nx-forge/src/executors/install/schema.d.tspackages/nx-forge/src/executors/install/schema.jsonpackages/nx-forge/src/executors/package/executor.tspackages/nx-forge/src/executors/package/lib/copy-forge-app-assets.tspackages/nx-forge/src/executors/package/lib/create-package-json.tspackages/nx-forge/src/executors/package/lib/patch-manifest-yml.tspackages/nx-forge/src/executors/package/lib/process-resource-dependencies.tspackages/nx-forge/src/generators/application/files/README.mdpackages/nx-forge/src/generators/application/files/manifest.ymlpackages/nx-forge/src/generators/application/files/tsconfig.app.jsonpackages/nx-forge/src/generators/application/generator.spec.tspackages/nx-forge/src/graph/create-nodes.tspackages/nx-forge/src/migrations/update-2-3-0/add-nx-forge-plugin.tspackages/nx-forge/src/migrations/update-2-3-0/remove-implicit-custom-ui-dependencies.tspackages/nx-forge/src/migrations/update-3-0-0-webpack-config-setup/update-3-0-0-webpack-config-setup.tspackages/nx-forge/src/shared/targets/get-install-config.tstools/docs/.eslintrc.jsontools/docs/eslint.config.mjstools/docs/src/lib/reference-docs.spec.tstools/scripts/start-local-registry.ts
💤 Files with no reviewable changes (22)
- packages/nx-forge/src/executors/install/schema.d.ts
- packages/nx-forge/src/executors/install/executor.ts
- .eslintignore
- packages/nx-forge/src/executors/build/schema.d.ts
- packages/nx-forge/src/migrations/update-2-3-0/add-nx-forge-plugin.ts
- packages/nx-forge/jest.config.ts
- packages/nx-forge/src/executors/package/lib/copy-forge-app-assets.ts
- packages/nx-forge/src/graph/create-nodes.ts
- .eslintrc.json
- packages/nx-forge/src/executors/build/lib/compile-webpack.ts
- packages/nx-forge/src/migrations/update-2-3-0/remove-implicit-custom-ui-dependencies.ts
- tools/docs/.eslintrc.json
- packages/nx-forge/src/executors/build/lib/normalize-options.ts
- packages/nx-forge/src/executors/build/executor.ts
- e2e/nx-forge-e2e/tests/build.executor.spec.ts
- packages/nx-forge/src/shared/targets/get-install-config.ts
- packages/nx-forge/src/executors/build/schema.json
- packages/nx-forge/executors.json
- packages/nx-forge/src/executors/install/schema.json
- packages/nx-forge/src/executors/package/lib/create-package-json.ts
- packages/nx-forge/src/executors/build/lib/generate-package-json.ts
- packages/nx-forge/.eslintrc.json
✅ Files skipped from review due to trivial changes (22)
- packages/nx-forge/project.json
- docs/guides/generating-a-forge-app.md
- docs/concepts/plugin-concepts.md
- packages/nx-forge/src/executors/package/lib/patch-manifest-yml.ts
- .gitignore
- packages/nx-forge/src/generators/application/files/manifest.yml
- docs/guides/migrating-to-package-executor.md
- packages/nx-forge/package.json
- .codex/config.toml
- docs/guides/adding-a-ui-kit-module.md
- e2e/nx-forge-e2e/tests/utils/generate-forge-app.ts
- docs/guides/getting-started.md
- packages/nx-forge/src/executors/package/lib/process-resource-dependencies.ts
- packages/nx-forge/src/generators/application/files/tsconfig.app.json
- tools/docs/eslint.config.mjs
- packages/nx-forge/src/generators/application/files/README.md
- eslint.config.mjs
- tools/docs/src/lib/reference-docs.spec.ts
- docs/.vitepress/grammars/jsonata.tmLanguage.json
- AGENTS.md
- packages/nx-forge/src/executors/package/executor.ts
- packages/nx-forge/src/generators/application/generator.spec.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- tools/scripts/start-local-registry.ts
- docs/.vitepress/config.mts
- docs/guides/migrating-to-inferred-tasks.md
- packages/nx-forge/src/migrations/update-3-0-0-webpack-config-setup/update-3-0-0-webpack-config-setup.ts
- docs/guides/transforming-the-manifest.md
- nx.json
- e2e/nx-forge-e2e/tests/basic-setup.spec.ts
- packages/nx-forge/eslint.config.mjs
Comment on lines
+47
to
+52
| (err, stdout, stderr) => { | ||
| if (!opts.silenceError && err) { | ||
| reject(err); | ||
| } | ||
| resolve({ stdout, stderr }); | ||
| } |
There was a problem hiding this comment.
Add early return after reject to prevent calling resolve.
When err is truthy and silenceError is false, both reject(err) and resolve({ stdout, stderr }) execute. While promises only settle once, calling both is a code smell.
🛠️ Proposed fix
(err, stdout, stderr) => {
if (!opts.silenceError && err) {
- reject(err);
+ return reject(err);
}
resolve({ stdout, stderr });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| (err, stdout, stderr) => { | |
| if (!opts.silenceError && err) { | |
| reject(err); | |
| } | |
| resolve({ stdout, stderr }); | |
| } | |
| (err, stdout, stderr) => { | |
| if (!opts.silenceError && err) { | |
| return reject(err); | |
| } | |
| resolve({ stdout, stderr }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/nx-forge-e2e/tests/utils/async-commands.ts` around lines 47 - 52, The
callback passed to the async command helper currently calls reject(err) then
continues to call resolve({ stdout, stderr }) which is confusing; update the
handler (the callback that receives (err, stdout, stderr) in
e2e/nx-forge-e2e/tests/utils/async-commands.ts) to return immediately after
calling reject (or use an else) when err is truthy and opts.silenceError is
false so resolve is not invoked thereafter; locate the block referencing
opts.silenceError, err, reject and resolve and add a `return` (or convert to `if
(...) { reject(err); return; }`) to stop execution.
Contributor
|
🎉 This PR is included in version 7.0.0-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Contributor
|
🎉 This PR is included in version 7.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.
BREAKING CHANGE: requires Nx 22, removed build and install executors
Closes #181
Summary by CodeRabbit
New Features
Breaking Changes
Documentation
Chores