Conversation
…command with --no-git-checks
📝 WalkthroughWalkthroughThis pull request updates CI/CD workflows, dependency versions, refactors the build system to use a new configuration loading mechanism for dependency-cruiser, and updates TypeScript configuration inheritance paths to use distributable versions from Changes
Possibly Related PRs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/npm-publish.yml (1)
56-66: Unquoted variable expansion of$BASE_CMDmay cause issues in edge cases.While the current value won't trigger word-splitting issues, it's a good shell hygiene practice to quote the expansion or use an array/function instead.
Suggested fix
- BASE_CMD="npx pnpm publish --ignore-scripts --no-git-checks" - - if [[ "$TAG" == *"-alpha."* ]]; then - $BASE_CMD --pre-dist-tag alpha - elif [[ "$TAG" == *"-beta."* ]]; then - $BASE_CMD --pre-dist-tag beta - else - $BASE_CMD - fi + publish() { + npx pnpm publish --ignore-scripts --no-git-checks "$@" + } + + if [[ "$TAG" == *"-alpha."* ]]; then + publish --pre-dist-tag alpha + elif [[ "$TAG" == *"-beta."* ]]; then + publish --pre-dist-tag beta + else + publish + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/npm-publish.yml around lines 56 - 66, The workflow unquoted the variable expansion BASE_CMD when executing it (and in the conditional branches), which can lead to word-splitting or globbing edge cases; update the script to safely invoke the prepared command by either quoting the expansion ("$BASE_CMD") when running it or, better, store the command as an array/func (e.g., base_cmd=(npx pnpm publish --ignore-scripts --no-git-checks)) and call it with "${base_cmd[@]}" and then append the --pre-dist-tag arg when TAG contains "-alpha." or "-beta." so BASE_CMD is executed safely in the alpha, beta, and default branches.src/build/build-circular-deps.ts (1)
31-31: Fragile type cast on cycle step extraction.
(step as { name: string }).nameassumes each cycle step has anameproperty. If thedependency-cruiserAPI changes the cycle step shape, this will throw at runtime without a helpful error. Consider adding a guard or using the library's own type for cycle steps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/build-circular-deps.ts` at line 31, The mapping of violation.cycle to extract names is using a fragile cast (step as { name: string }).name; update the extraction in build-circular-deps.ts to safely access a name field (or the library's official cycle-step type) by checking for step && typeof (step as any).name === 'string' before reading it, falling back to a safe representation (e.g., step.path or String(step)) when name is missing, or import and use dependency-cruiser’s CycleStep type to type the array and access the property safely; ensure the variable cyclePath still becomes an array of strings even when some steps lack name.
🤖 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/npm-publish.yml:
- Around line 54-66: The publish step "Publish to NPM (OIDC Auth)" will fail
without an auth token; update the step that builds BASE_CMD and runs the publish
by either exporting NODE_AUTH_TOKEN from your secret (e.g., set env:
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} so pnpm can use classic token auth)
or, if you intend OIDC provenance, append --provenance to BASE_CMD and ensure
the action obtains and exposes the OIDC token expected by npm; adjust the
conditional branches that call $BASE_CMD (and the TAG handling) so they call the
chosen authenticated BASE_CMD consistently.
In `@src/build/build-circular-deps.ts`:
- Around line 17-25: Change the incorrect type cast of the cruise() result from
Partial<ICruiseResult> to IReporterOutput: replace the cast assigned to
cruiseResult (currently "result as Partial<ICruiseResult>") with "result as
IReporterOutput" and add IReporterOutput to the imports from dependency-cruiser
if missing; keep the rest of the logic that extracts violations and builds
circularViolations (the variables violations and circularViolations should
remain unchanged).
---
Nitpick comments:
In @.github/workflows/npm-publish.yml:
- Around line 56-66: The workflow unquoted the variable expansion BASE_CMD when
executing it (and in the conditional branches), which can lead to word-splitting
or globbing edge cases; update the script to safely invoke the prepared command
by either quoting the expansion ("$BASE_CMD") when running it or, better, store
the command as an array/func (e.g., base_cmd=(npx pnpm publish --ignore-scripts
--no-git-checks)) and call it with "${base_cmd[@]}" and then append the
--pre-dist-tag arg when TAG contains "-alpha." or "-beta." so BASE_CMD is
executed safely in the alpha, beta, and default branches.
In `@src/build/build-circular-deps.ts`:
- Line 31: The mapping of violation.cycle to extract names is using a fragile
cast (step as { name: string }).name; update the extraction in
build-circular-deps.ts to safely access a name field (or the library's official
cycle-step type) by checking for step && typeof (step as any).name === 'string'
before reading it, falling back to a safe representation (e.g., step.path or
String(step)) when name is missing, or import and use dependency-cruiser’s
CycleStep type to type the array and access the property safely; ensure the
variable cyclePath still becomes an array of strings even when some steps lack
name.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.github/workflows/node.js-ci.yml.github/workflows/npm-publish.ymlfiles/empty-project/package.jsonfiles/empty-project/tsconfig.base.jsonfiles/empty-project/tsconfig.browser.jsonfiles/empty-project/tsconfig.jsonfiles/empty-project/tsconfig.module.jsonfiles/empty-project/tsconfig.types.jsonfiles/empty-project/tsconfig.umd.jsonpackage.jsonsrc/build/build-circular-deps.tssrc/build/build-eslint.tssrc/build/build-tsc.tssrc/build/build.ts
| - name: Publish to NPM (OIDC Auth) | ||
| run: | | ||
| TAG="${GITHUB_REF#refs/tags/}" | ||
|
|
||
| BASE_CMD="npx pnpm publish --ignore-scripts --no-git-checks" | ||
|
|
||
| if [[ "$TAG" == *"-alpha."* ]]; then | ||
| $BASE_CMD --pre-dist-tag alpha | ||
| elif [[ "$TAG" == *"-beta."* ]]; then | ||
| $BASE_CMD --pre-dist-tag beta | ||
| else | ||
| $BASE_CMD | ||
| fi |
There was a problem hiding this comment.
Missing NODE_AUTH_TOKEN environment variable — publish will fail.
The workflow sets up OIDC permissions (id-token: write) but the publish step neither passes NODE_AUTH_TOKEN nor uses --provenance to leverage OIDC-based npm authentication. Without one of these, pnpm publish will fail with a 401/403 from the npm registry.
If OIDC provenance-based publishing is intended, add --provenance and the token. If classic token auth is intended, supply the secret:
Proposed fix (OIDC provenance)
- name: Publish to NPM (OIDC Auth)
run: |
TAG="${GITHUB_REF#refs/tags/}"
- BASE_CMD="npx pnpm publish --ignore-scripts --no-git-checks"
+ BASE_CMD="npx pnpm publish --ignore-scripts --no-git-checks --provenance"
if [[ "$TAG" == *"-alpha."* ]]; then
$BASE_CMD --pre-dist-tag alpha
elif [[ "$TAG" == *"-beta."* ]]; then
$BASE_CMD --pre-dist-tag beta
else
$BASE_CMD
fi
+ env:
+ NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}📝 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.
| - name: Publish to NPM (OIDC Auth) | |
| run: | | |
| TAG="${GITHUB_REF#refs/tags/}" | |
| BASE_CMD="npx pnpm publish --ignore-scripts --no-git-checks" | |
| if [[ "$TAG" == *"-alpha."* ]]; then | |
| $BASE_CMD --pre-dist-tag alpha | |
| elif [[ "$TAG" == *"-beta."* ]]; then | |
| $BASE_CMD --pre-dist-tag beta | |
| else | |
| $BASE_CMD | |
| fi | |
| - name: Publish to NPM (OIDC Auth) | |
| run: | | |
| TAG="${GITHUB_REF#refs/tags/}" | |
| BASE_CMD="npx pnpm publish --ignore-scripts --no-git-checks --provenance" | |
| if [[ "$TAG" == *"-alpha."* ]]; then | |
| $BASE_CMD --pre-dist-tag alpha | |
| elif [[ "$TAG" == *"-beta."* ]]; then | |
| $BASE_CMD --pre-dist-tag beta | |
| else | |
| $BASE_CMD | |
| fi | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/npm-publish.yml around lines 54 - 66, The publish step
"Publish to NPM (OIDC Auth)" will fail without an auth token; update the step
that builds BASE_CMD and runs the publish by either exporting NODE_AUTH_TOKEN
from your secret (e.g., set env: NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} so
pnpm can use classic token auth) or, if you intend OIDC provenance, append
--provenance to BASE_CMD and ensure the action obtains and exposes the OIDC
token expected by npm; adjust the conditional branches that call $BASE_CMD (and
the TAG handling) so they call the chosen authenticated BASE_CMD consistently.
| const result = await cruise([srcPath], { | ||
| ...cruiseOptions.options, | ||
| ruleSet: { | ||
| forbidden: extendedConfig.forbidden ?? [], | ||
| forbidden: cruiseOptions.forbidden ?? [], | ||
| }, | ||
| }; | ||
| } else { | ||
| console.log("No .dependency-cruiser.js found, applying default circular check."); | ||
|
|
||
| cruiseOptions.ruleSet = { | ||
| forbidden: [ | ||
| { | ||
| name: "no-circular", | ||
| severity: "error", | ||
| from: {}, | ||
| to: { circular: true }, | ||
| }, | ||
| ], | ||
| }; | ||
| } | ||
|
|
||
| const result: unknown = await cruise([srcPath], cruiseOptions), | ||
| }), | ||
| cruiseResult = result as Partial<ICruiseResult>, | ||
| violations: IViolation[] = cruiseResult.summary?.violations ?? [], | ||
| circularViolations = violations.filter((violation: IViolation) => violation.rule.name === "no-circular"); | ||
| circularViolations = violations.filter(violation => violation.rule.name === "no-circular"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
dependency-cruiser cruise function async v17 API
💡 Result:
In dependency-cruiser v17, the programmatic API is ESM-only and cruise() is async, so you call it with await and it returns a Promise<IReporterOutput>. [1]
cruise() call shape (v17)
import { cruise } from "dependency-cruiser";
const result = await cruise(
["src"], // filesOrDirectories
cruiseOptions, // (optional) ICruiseOptions
resolveOptions, // (optional) webpack/enhanced-resolve-style options
{ tsConfig } // (optional) transpilation options object (supports tsConfig, babelConfig)
);This parameter structure (notably the 4th param as an object that can include tsConfig and/or babelConfig) is the “new” signature introduced when the API became async/ESM. [1][2]
Notes that commonly trip people up
- Validation is off by default in the API; set
cruiseOptions.validate = trueand pass aruleSetif you want rule violations in the output. [1] - There was a specific fix for “Using the API but tsconfig does not work” in the v17.3.0 line—so if TS path mapping etc. seems ignored, ensure you’re on ≥ 17.3.0. [3]
Sources:
[1] dependency-cruiser doc/api.md (usage examples + options) (raw.githubusercontent.com)
[2] Breaking changes note (API is async/ESM; cruise signature change; 4th param object for tsConfig/babelConfig) (github.com)
[3] v17.3.0 release notes (fix: “Using the API but tsconfig does not work”) (github.com)
Citations:
- 1: https://raw.githubusercontent.com/sverweij/dependency-cruiser/main/doc/api.md
- 2: Breaking changes in dependency-cruiser v13 sverweij/dependency-cruiser#794
- 3: https://github.com/sverweij/dependency-cruiser/releases?utm_source=openai
🏁 Script executed:
# Check the package.json to see the version of dependency-cruiser
find . -name "package.json" -type f | head -5 | xargs grep -l "dependency-cruiser" 2>/dev/null
# Also check the specific file mentioned in the review
cat -n src/build/build-circular-deps.ts 2>/dev/null | head -40Repository: tsparticles/cli
Length of output: 1787
🏁 Script executed:
# Check if there are type definitions or other imports that show the actual types
rg "IReporterOutput|ICruiseResult" src/ --type ts --type tsx -C 2Repository: tsparticles/cli
Length of output: 86
🏁 Script executed:
# Search for type definitions with just ts files
rg "IReporterOutput|ICruiseResult" src/ -C 2Repository: tsparticles/cli
Length of output: 761
🏁 Script executed:
# Check the package.json to see the exact version of dependency-cruiser being used
cat package.json | grep -A 2 -B 2 "dependency-cruiser"Repository: tsparticles/cli
Length of output: 198
Update type cast to use the correct v17 return type.
The await on line 17 is correct—cruise() is async in dependency-cruiser v17. However, the return type in v17 is IReporterOutput, not ICruiseResult. Update the type cast on line 23 from Partial<ICruiseResult> to IReporterOutput, and add it to the imports if not already present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/build/build-circular-deps.ts` around lines 17 - 25, Change the incorrect
type cast of the cruise() result from Partial<ICruiseResult> to IReporterOutput:
replace the cast assigned to cruiseResult (currently "result as
Partial<ICruiseResult>") with "result as IReporterOutput" and add
IReporterOutput to the imports from dependency-cruiser if missing; keep the rest
of the logic that extracts violations and builds circularViolations (the
variables violations and circularViolations should remain unchanged).
1 participant
Summary by CodeRabbit