Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSwitched many packages from Webpack to esbuild/ESM outputs, migrated numerous Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
libs/sdk/eslint.config.ts (1)
13-18: Consider adding test/e2e exclusions for consistency.Other ESLint configs in this PR (e.g.,
libs/converter-openapi/eslint.config.ts,libs/backend-apisix/eslint.config.ts) add{projectRoot}/test/**/*and{projectRoot}/e2e/**/*toignoredFiles. This file only has thevitest.configchange but not the test path exclusions.If the sdk package has test files, consider adding the same exclusions for consistency:
ignoredFiles: [ '{projectRoot}/eslint.config.{js,cjs,mjs,ts,cts,mts}', '{projectRoot}/vitest.config.{js,ts,mjs,mts}', + '{projectRoot}/test/**/*', + '{projectRoot}/e2e/**/*', ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/eslint.config.ts` around lines 13 - 18, In libs/sdk/eslint.config.ts update the ignoredFiles array (the ignoredFiles entry currently containing '{projectRoot}/eslint.config.{js,cjs,mjs,ts,cts,mts}' and '{projectRoot}/vitest.config.{js,ts,mjs,mts}') to also include the test exclusions '{projectRoot}/test/**/*' and '{projectRoot}/e2e/**/*' so it matches the other packages' ESLint configs; keep ignoredDependencies: ['tslib'] unchanged.libs/converter-openapi/package.json (1)
4-15: Inconsistency betweenexportsandmain/module/typesfields.The
main,module, andtypesfields point to built artifacts in./dist/:"types": "./dist/index.d.ts", "module": "./dist/main.js", "main": "./dist/main.js",However, the
exportsfield still points to source files in./src/:"exports": { ".": { "types": "./src/index.ts", "import": "./src/index.ts", "default": "./src/index.ts" }, ... }Since
exportstakes precedence overmain/modulein modern Node.js resolution, consumers will resolve to source TypeScript files rather than built artifacts. This may be intentional for monorepo development (since"private": true), but if consistency is desired:Proposed fix to align exports with dist outputs
"exports": { ".": { - "types": "./src/index.ts", - "import": "./src/index.ts", - "default": "./src/index.ts" + "types": "./dist/index.d.ts", + "import": "./dist/main.js", + "default": "./dist/main.js" }, "./package.json": "./package.json" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/converter-openapi/package.json` around lines 4 - 15, The exports field is pointing at source files while main/module/types reference built artifacts; update the exports mapping (the "." entry) to reference the dist outputs instead of src — set "types" to "./dist/index.d.ts", "import" to "./dist/main.js" and "default" to "./dist/main.js" (leave the "./package.json" export as-is) so Node resolution uses the built artifacts consistently with the main/module/types fields.libs/backend-api7/package.json (1)
48-72: SamedependsOnrecommendation as other libraries.Add
dependsOn: ["^build"]to ensure workspace dependencies are built first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-api7/package.json` around lines 48 - 72, The build target in package.json (the "build" executor block for libs/backend-api7) lacks a dependsOn entry, so workspace dependencies may not be built before this package; add a dependsOn: ["^build"] property inside the same "build" configuration object (the one containing "executor": "@nx/esbuild:esbuild", "options", and "configurations") so that the build target depends on parent workspace packages being built first.libs/backend-apisix-standalone/package.json (1)
48-73: SamedependsOnconsideration applies here.Like
backend-apisix, this library depends on workspace packages (@api7/adc-sdk) and would benefit fromdependsOn: ["^build"]to ensure correct build ordering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-apisix-standalone/package.json` around lines 48 - 73, Add a dependsOn entry to the build target in libs/backend-apisix-standalone package.json so the build waits for dependent workspace packages; specifically modify the "build" target (the object with "executor": "@nx/esbuild:esbuild") to include "dependsOn": ["^build"] to ensure packages like `@api7/adc-sdk` are built first.libs/backend-apisix/package.json (2)
47-72: MissingdependsOnconfiguration for workspace dependency build ordering.This library depends on
@api7/adc-sdk(workspace package). WithoutdependsOn: ["^build"]on the build target, Nx won't guarantee that dependencies are built first. If the build relies on the dist artifacts from@api7/adc-sdk, this could cause intermittent build failures.Proposed fix
"build": { "executor": "@nx/esbuild:esbuild", + "dependsOn": ["^build"], "outputs": [ "{options.outputPath}" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-apisix/package.json` around lines 47 - 72, The build target in this package.json (the "build" target for libs/backend-apisix) lacks a dependsOn entry so Nx won't enforce building workspace deps like `@api7/adc-sdk` first; update the "build" target to include dependsOn: ["^build"] so dependent packages are built prior to this library (modify the build target block in libs/backend-apisix/package.json to add the dependsOn key).
5-14: Inconsistent type declaration paths between top-level and exports fields.The top-level
typesfield points to./dist/index.d.ts(built output) whileexports["."].typespoints to./src/index.ts(source). This dual mapping is a common monorepo pattern—source paths for development tooling and dist paths for consumers—but verify that your tooling (TypeScript, bundlers) resolves to the intended path in each scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-apisix/package.json` around lines 5 - 14, The package.json has inconsistent type entry points: top-level "types" points to "./dist/index.d.ts" but exports["."].types points to "./src/index.ts"; update the exports mapping so exports["."].types references the built declaration (e.g., "./dist/index.d.ts") and ensure exports["."].import and exports["."].default point to the consumed build (e.g., "./dist/main.js") to match "main" and "module" and avoid tooling resolution mismatches.apps/cli/package.json (1)
59-63: Consider using.cjsextension for CommonJS output in an ESM package.The build outputs CJS format (
"format": ["cjs"]) but uses.jsextension. In a"type": "module"package,.jsfiles are treated as ESM by Node.js. While esbuild may add appropriate markers, explicitly usingmain.cjsmakes the module type unambiguous and aligns with the AI summary's mention of.cjsextensions.Proposed fix
"outputPath": "dist/apps/cli", - "outputFileName": "main.js", + "outputFileName": "main.cjs", "tsConfig": "apps/cli/tsconfig.app.json",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/package.json` around lines 59 - 63, The package build is producing CommonJS ("format": ["cjs"]) but the outputFileName is "main.js", which is ambiguous in a "type": "module" package; update the build config so that when "format" includes "cjs" the "outputFileName" uses a .cjs extension (e.g., change outputFileName from "main.js" to "main.cjs") to make the module type explicit; locate and update the outputFileName field in the CLI package build config where "format": ["cjs"] is declared (reference symbols: "outputFileName" and "format") and ensure any downstream references to the binary or package main point to the new .cjs filename.libs/differ/package.json (1)
34-59: Consistent with other library builds; samedependsOnrecommendation applies.The esbuild configuration follows the same pattern as other libraries. Consider adding
dependsOn: ["^build"]to ensure@api7/adc-sdkis built first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/differ/package.json` around lines 34 - 59, The package.json "build" target for the differ lib currently lacks an explicit dependsOn; add a dependsOn entry to the build target (the existing "build" object using executor "@nx/esbuild:esbuild") to ensure dependent workspace libs are built first, e.g., set dependsOn to ["^build"] so that `@api7/adc-sdk` and other upstream libs are built before this library's build runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/package.json`:
- Around line 91-96: The "export-schema" NX target currently runs "ts-node
apps/cli/src/linter/exporter.ts" which will fail because the ESM imports in
apps/cli/src/linter/exporter.ts require ts-node's ESM loader; update the
target's command to invoke Node with the ts-node ESM loader (e.g., use node
--loader ts-node/esm ...) so the file's ESM imports parse correctly and ensure
ts-node is a dependency; modify the "export-schema" options.command accordingly.
- Around line 50-78: The build target in apps/cli's project configuration (the
"build" object) must declare a dependsOn entry to ensure workspace libs are
built first; add "dependsOn": ["^build"] to the same build target object so Nx
will run the root/ancestor build for dependent projects (refer to the "build"
target in apps/cli/package.json) before building the CLI.
---
Nitpick comments:
In `@apps/cli/package.json`:
- Around line 59-63: The package build is producing CommonJS ("format": ["cjs"])
but the outputFileName is "main.js", which is ambiguous in a "type": "module"
package; update the build config so that when "format" includes "cjs" the
"outputFileName" uses a .cjs extension (e.g., change outputFileName from
"main.js" to "main.cjs") to make the module type explicit; locate and update the
outputFileName field in the CLI package build config where "format": ["cjs"] is
declared (reference symbols: "outputFileName" and "format") and ensure any
downstream references to the binary or package main point to the new .cjs
filename.
In `@libs/backend-api7/package.json`:
- Around line 48-72: The build target in package.json (the "build" executor
block for libs/backend-api7) lacks a dependsOn entry, so workspace dependencies
may not be built before this package; add a dependsOn: ["^build"] property
inside the same "build" configuration object (the one containing "executor":
"@nx/esbuild:esbuild", "options", and "configurations") so that the build target
depends on parent workspace packages being built first.
In `@libs/backend-apisix-standalone/package.json`:
- Around line 48-73: Add a dependsOn entry to the build target in
libs/backend-apisix-standalone package.json so the build waits for dependent
workspace packages; specifically modify the "build" target (the object with
"executor": "@nx/esbuild:esbuild") to include "dependsOn": ["^build"] to ensure
packages like `@api7/adc-sdk` are built first.
In `@libs/backend-apisix/package.json`:
- Around line 47-72: The build target in this package.json (the "build" target
for libs/backend-apisix) lacks a dependsOn entry so Nx won't enforce building
workspace deps like `@api7/adc-sdk` first; update the "build" target to include
dependsOn: ["^build"] so dependent packages are built prior to this library
(modify the build target block in libs/backend-apisix/package.json to add the
dependsOn key).
- Around line 5-14: The package.json has inconsistent type entry points:
top-level "types" points to "./dist/index.d.ts" but exports["."].types points to
"./src/index.ts"; update the exports mapping so exports["."].types references
the built declaration (e.g., "./dist/index.d.ts") and ensure exports["."].import
and exports["."].default point to the consumed build (e.g., "./dist/main.js") to
match "main" and "module" and avoid tooling resolution mismatches.
In `@libs/converter-openapi/package.json`:
- Around line 4-15: The exports field is pointing at source files while
main/module/types reference built artifacts; update the exports mapping (the "."
entry) to reference the dist outputs instead of src — set "types" to
"./dist/index.d.ts", "import" to "./dist/main.js" and "default" to
"./dist/main.js" (leave the "./package.json" export as-is) so Node resolution
uses the built artifacts consistently with the main/module/types fields.
In `@libs/differ/package.json`:
- Around line 34-59: The package.json "build" target for the differ lib
currently lacks an explicit dependsOn; add a dependsOn entry to the build target
(the existing "build" object using executor "@nx/esbuild:esbuild") to ensure
dependent workspace libs are built first, e.g., set dependsOn to ["^build"] so
that `@api7/adc-sdk` and other upstream libs are built before this library's build
runs.
In `@libs/sdk/eslint.config.ts`:
- Around line 13-18: In libs/sdk/eslint.config.ts update the ignoredFiles array
(the ignoredFiles entry currently containing
'{projectRoot}/eslint.config.{js,cjs,mjs,ts,cts,mts}' and
'{projectRoot}/vitest.config.{js,ts,mjs,mts}') to also include the test
exclusions '{projectRoot}/test/**/*' and '{projectRoot}/e2e/**/*' so it matches
the other packages' ESLint configs; keep ignoredDependencies: ['tslib']
unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d665a43b-e798-4865-ae36-aa4b2e4af6b8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
README.mdapps/cli/eslint.config.tsapps/cli/node-sea.jsonapps/cli/package.jsonapps/cli/project.jsonapps/cli/src/command/ingress-server.command.tsapps/cli/src/main.tsapps/cli/webpack.config.jslibs/backend-api7/eslint.config.tslibs/backend-api7/package.jsonlibs/backend-apisix-standalone/eslint.config.tslibs/backend-apisix-standalone/package.jsonlibs/backend-apisix/eslint.config.tslibs/backend-apisix/package.jsonlibs/converter-openapi/eslint.config.tslibs/converter-openapi/package.jsonlibs/differ/eslint.config.tslibs/differ/package.jsonlibs/sdk/eslint.config.tslibs/sdk/package.jsonlibs/tools/src/docker/Dockerfilenx.jsonpackage.jsonpnpm-workspace.yamltsconfig.base.json
💤 Files with no reviewable changes (4)
- apps/cli/src/main.ts
- apps/cli/eslint.config.ts
- apps/cli/project.json
- apps/cli/webpack.config.js
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/cli/package.json (1)
50-55: 🛠️ Refactor suggestion | 🟠 MajorAdd
dependsOn: ["^build"]to ensure workspace libraries are built first.The CLI depends on workspace packages (
@api7/adc-sdk,@api7/adc-backend-*, etc.) that now have their own esbuild targets. WithoutdependsOn, Nx won't guarantee these libraries are built before the CLI, potentially causing build failures when the CLI tries to import from unbuilt workspace packages.Proposed fix
"build": { "executor": "@nx/esbuild:esbuild", + "dependsOn": ["^build"], "outputs": [ "{options.outputPath}" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/package.json` around lines 50 - 55, The build target in apps/cli package.json uses the "@nx/esbuild:esbuild" executor but lacks a dependsOn declaration, so add "dependsOn": ["^build"] to the same build target object to ensure workspace libraries' build targets run first; update the build target (the object containing "executor": "@nx/esbuild:esbuild", "outputs": ["{options.outputPath}"], "defaultConfiguration": "production") to include the dependsOn array.
🧹 Nitpick comments (2)
libs/backend-api7/package.json (1)
24-24: Consider moving@types/json-schemato devDependencies.Type definition packages (
@types/*) are typically dev dependencies since they're only needed at compile time, not runtime. Unless this package re-exports types that consumers need,@types/json-schemashould be indevDependencies.Proposed fix
"devDependencies": { "@api7/adc-differ": "workspace:*", "@types/lodash-es": "catalog:", + "@types/json-schema": "^7.0.15", "@types/semver": "catalog:", "vitest": "catalog:" }, "dependencies": { "@api7/adc-sdk": "workspace:*", - "@types/json-schema": "^7.0.15", "axios": "catalog:",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-api7/package.json` at line 24, The package.json currently lists "@types/json-schema" in dependencies; move it to devDependencies because it's a TypeScript type-only package (unless this package intentionally re-exports those types to consumers). Remove the "@types/json-schema" entry from the "dependencies" section and add the same version string under "devDependencies", then run your package manager (npm/yarn/pnpm) to update lockfiles; confirm no public API re-exports require keeping it in dependencies.libs/backend-api7/e2e/support/utils.ts (1)
91-94: Thestringcast may fail at runtime for non-string fields.The function now assumes
fieldalways contains a string value. If a numeric or other non-string field is passed, callinglocaleCompareon a cast non-string could produce unexpected sorting results. Consider adding a runtime guard or usingString()for safer coercion.💡 Safer string coercion
export const sortResult = <T>(result: Array<T>, field: string) => structuredClone(result).sort((a: any, b: any) => - (a[field] as string).localeCompare(b[field] as string), + String(a[field]).localeCompare(String(b[field])), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-api7/e2e/support/utils.ts` around lines 91 - 94, The comparator in sortResult assumes the property is a string and naively casts to string, which can fail for numbers or undefined; update sortResult to coerce values safely (e.g., use String(a[field] ?? '') and String(b[field] ?? '') or add a runtime type guard) before calling localeCompare so non-string fields and missing values are handled deterministically; keep the function name sortResult and param field unchanged while only changing the sort comparator logic to perform safe coercion and fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/package.json`:
- Line 59: The package.json esbuild setting "outputFileName" currently yields
"main.js" which mismatches the SEA config that expects the built artifact named
"main.cjs"; update the "outputFileName" value to "main.cjs" (or change the SEA
config to match) so the CLI build artifact name produced by the esbuild config
and the SEA configuration agree; search for "outputFileName" in package.json and
the SEA configuration entry that references "main.cjs" to ensure both use the
same filename.
In `@libs/backend-api7/e2e/misc.e2e-spec.ts`:
- Around line 62-64: There is a duplicate assertion calling
expect(result.services?.[0]).toMatchObject(service) twice; edit the test in
misc.e2e-spec.ts to remove the redundant assertion so only one
expect(result.services?.[0]).toMatchObject(service) remains, keeping the
subsequent expect(result.services?.[0].routes?.[0]).toMatchObject(route) intact;
this will eliminate the duplicate check while preserving coverage of the service
and route assertions.
---
Duplicate comments:
In `@apps/cli/package.json`:
- Around line 50-55: The build target in apps/cli package.json uses the
"@nx/esbuild:esbuild" executor but lacks a dependsOn declaration, so add
"dependsOn": ["^build"] to the same build target object to ensure workspace
libraries' build targets run first; update the build target (the object
containing "executor": "@nx/esbuild:esbuild", "outputs":
["{options.outputPath}"], "defaultConfiguration": "production") to include the
dependsOn array.
---
Nitpick comments:
In `@libs/backend-api7/e2e/support/utils.ts`:
- Around line 91-94: The comparator in sortResult assumes the property is a
string and naively casts to string, which can fail for numbers or undefined;
update sortResult to coerce values safely (e.g., use String(a[field] ?? '') and
String(b[field] ?? '') or add a runtime type guard) before calling localeCompare
so non-string fields and missing values are handled deterministically; keep the
function name sortResult and param field unchanged while only changing the sort
comparator logic to perform safe coercion and fallback.
In `@libs/backend-api7/package.json`:
- Line 24: The package.json currently lists "@types/json-schema" in
dependencies; move it to devDependencies because it's a TypeScript type-only
package (unless this package intentionally re-exports those types to consumers).
Remove the "@types/json-schema" entry from the "dependencies" section and add
the same version string under "devDependencies", then run your package manager
(npm/yarn/pnpm) to update lockfiles; confirm no public API re-exports require
keeping it in dependencies.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7fc2918-df4a-4e55-93f3-ce8230b8f962
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
apps/cli/package.jsonapps/cli/src/command/convert.command.tsapps/cli/src/command/helper.tsapps/cli/src/command/ingress-sync.command.tsapps/cli/src/command/utils.tsapps/cli/src/server/sync.tsapps/cli/src/utils/listr.tsapps/cli/tsconfig.jsonlibs/backend-api7/e2e/label-selector.e2e-spec.tslibs/backend-api7/e2e/misc.e2e-spec.tslibs/backend-api7/e2e/resources/consumer.e2e-spec.tslibs/backend-api7/e2e/resources/route.e2e-spec.tslibs/backend-api7/e2e/resources/service-upstream.e2e-spec.tslibs/backend-api7/e2e/support/utils.tslibs/backend-api7/e2e/sync-and-dump-1.e2e-spec.tslibs/backend-api7/e2e/sync-and-dump-2.e2e-spec.tslibs/backend-api7/package.jsonlibs/backend-api7/src/fetcher.tslibs/backend-api7/src/index.tslibs/backend-api7/src/transformer.tslibs/backend-api7/test/fetcher.spec.tslibs/backend-apisix-standalone/package.jsonlibs/backend-apisix-standalone/src/operator.tslibs/backend-apisix-standalone/src/transformer.tslibs/backend-apisix/e2e/sync-and-dump-1.e2e-spec.tslibs/backend-apisix/package.jsonlibs/backend-apisix/src/fetcher.tslibs/backend-apisix/src/operator.tslibs/backend-apisix/src/transformer.tslibs/converter-openapi/package.jsonlibs/converter-openapi/src/extension.tslibs/converter-openapi/src/index.tslibs/differ/package.jsonlibs/differ/src/differv3.tslibs/sdk/package.jsonlibs/sdk/src/backend/utils.tslibs/sdk/src/core/schema.tslibs/sdk/src/utils.tspnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- apps/cli/tsconfig.json
✅ Files skipped from review due to trivial changes (23)
- libs/converter-openapi/src/extension.ts
- libs/sdk/src/backend/utils.ts
- libs/backend-api7/e2e/resources/service-upstream.e2e-spec.ts
- apps/cli/src/command/helper.ts
- libs/backend-apisix-standalone/src/operator.ts
- libs/sdk/src/utils.ts
- apps/cli/src/command/convert.command.ts
- libs/backend-apisix/src/fetcher.ts
- libs/differ/src/differv3.ts
- libs/backend-apisix/src/transformer.ts
- libs/backend-api7/src/transformer.ts
- libs/sdk/src/core/schema.ts
- apps/cli/src/utils/listr.ts
- libs/backend-api7/src/fetcher.ts
- libs/backend-api7/src/index.ts
- libs/backend-apisix/src/operator.ts
- libs/backend-apisix-standalone/src/transformer.ts
- libs/backend-api7/test/fetcher.spec.ts
- libs/converter-openapi/src/index.ts
- apps/cli/src/command/ingress-sync.command.ts
- apps/cli/src/command/utils.ts
- apps/cli/src/server/sync.ts
- pnpm-workspace.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/sdk/package.json
- libs/differ/package.json
Description
Checklist
Summary by CodeRabbit