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:
WalkthroughThis pull request migrates the kg-utils package from CommonJS JavaScript to TypeScript with ES modules. Key changes include: converting source files (index.js, slugify.js) to TypeScript equivalents (src/index.ts, src/slugify.ts), adding a SlugifyOptions interface to the slugify function, updating the ESLint configuration to support TypeScript with tseslint.config wrapper, adding a tsconfig.json for TypeScript compilation, updating package.json to point to build artifacts instead of source files, converting test files from CommonJS to ES module syntax, moving test global setup from JavaScript to a new TypeScript utility file, and establishing build/prepare/pretest scripts using the TypeScript compiler. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Member
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
kevinansfield
force-pushed
the
ts/kg-utils
branch
from
b8beafa to
03fa69c
Compare
Member
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/kg-utils/tsconfig.json (1)
6-19:pretestis not type-checking the migrated.tstests.With
"rootDir": "src"and"include": ["src/**/*"], everything undertest/**/*.tsis outside thetscprogram. Those files only transpile throughtsxat runtime, so TypeScript errors in the new test suite will never be caught during pretest. Consider adding atsconfig.test.jsonwithtsc -p tsconfig.test.json --noEmitto catch type errors in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-utils/tsconfig.json` around lines 6 - 19, The tsconfig currently sets "rootDir": "src" and "include": ["src/**/*"], so test files under test/**/*.ts are excluded from tsc and type errors aren't caught during pretest; create a separate tsconfig.test.json that includes your test folder (e.g., "include": ["src/**/*", "test/**/*.ts"]) and configure any necessary compilerOptions (or extend the base tsconfig) and update the pretest script to run tsc -p tsconfig.test.json --noEmit so tests are type-checked without emitting build artifacts.packages/kg-utils/test/slugify.test.ts (1)
2-2: Add one smoke test through the public entrypoint.These tests import the implementation file directly, so they won’t catch a broken
src/index.tsre-export or package-entry wiring—the main surface this PR is changing. I’d add at least one small test that importsslugifyfrom the public entrypoint as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-utils/test/slugify.test.ts` at line 2, Add a smoke test that imports the exported slugify from the package public entrypoint instead of the implementation file so the re-export/wiring is verified; create a new test (or update packages/kg-utils/test/slugify.test.ts) to import slugify via the package root (the module that re-exports slugify) and assert a simple transformation (e.g., slugify("Hello World") === "hello-world"), ensuring the test references the public export name slugify to catch any broken re-exports from index.
🤖 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/kg-utils/eslint.config.mjs`:
- Around line 23-29: Replace the current spread of only
ghostPlugin.configs['ts-test'].rules with the full preset so env, parserOptions,
and extends from ts-test are preserved: spread ghostPlugin.configs['ts-test']
into the file-specific config (e.g., files: ['test/**/*.ts'],
...ghostPlugin.configs['ts-test']) and then merge/override its rules by
providing a rules object that starts with
...ghostPlugin.configs['ts-test'].rules and then sets
'ghost/mocha/no-global-tests', 'ghost/mocha/handle-done-callback',
'ghost/mocha/no-mocha-arrows', and 'ghost/mocha/max-top-level-suites' to 'off'.
---
Nitpick comments:
In `@packages/kg-utils/test/slugify.test.ts`:
- Line 2: Add a smoke test that imports the exported slugify from the package
public entrypoint instead of the implementation file so the re-export/wiring is
verified; create a new test (or update packages/kg-utils/test/slugify.test.ts)
to import slugify via the package root (the module that re-exports slugify) and
assert a simple transformation (e.g., slugify("Hello World") === "hello-world"),
ensuring the test references the public export name slugify to catch any broken
re-exports from index.
In `@packages/kg-utils/tsconfig.json`:
- Around line 6-19: The tsconfig currently sets "rootDir": "src" and "include":
["src/**/*"], so test files under test/**/*.ts are excluded from tsc and type
errors aren't caught during pretest; create a separate tsconfig.test.json that
includes your test folder (e.g., "include": ["src/**/*", "test/**/*.ts"]) and
configure any necessary compilerOptions (or extend the base tsconfig) and update
the pretest script to run tsc -p tsconfig.test.json --noEmit so tests are
type-checked without emitting build artifacts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53ba0c28-2726-4115-a041-18047b29020c
📒 Files selected for processing (15)
packages/kg-utils/.gitignorepackages/kg-utils/eslint.config.mjspackages/kg-utils/index.jspackages/kg-utils/lib/kg-utils.jspackages/kg-utils/package.jsonpackages/kg-utils/src/index.tspackages/kg-utils/src/slugify.tspackages/kg-utils/test/slugify.test.tspackages/kg-utils/test/utils/assertions.jspackages/kg-utils/test/utils/assertions.tspackages/kg-utils/test/utils/index.jspackages/kg-utils/test/utils/index.tspackages/kg-utils/test/utils/overrides.jspackages/kg-utils/test/utils/overrides.tspackages/kg-utils/tsconfig.json
💤 Files with no reviewable changes (5)
- packages/kg-utils/test/utils/index.js
- packages/kg-utils/test/utils/overrides.js
- packages/kg-utils/lib/kg-utils.js
- packages/kg-utils/index.js
- packages/kg-utils/test/utils/assertions.js
Member
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
kevinansfield
force-pushed
the
ts/kg-utils
branch
from
e805989 to
4e71d9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/kg-utils/eslint.config.mjs`:
- Around line 5-16: Add a new lint block to the flat config returned by
tseslint.config that targets .mjs files (e.g., files: ['**/*.mjs']) and set
languageOptions.globals to include Node globals (use the globals package to
spread globals.node) so ESLint v9 won’t error on /* eslint-env */ comments;
modify the object list passed into tseslint.config (near the existing {files:
['**/*.ts'], ...} block and plugins: {ghost: ghostPlugin}) to include this
dedicated .mjs entry before or after the TS block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15da54dd-b9c4-4f44-a43d-ee7585731cc8
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
packages/kg-utils/.gitignorepackages/kg-utils/eslint.config.mjspackages/kg-utils/index.jspackages/kg-utils/lib/kg-utils.jspackages/kg-utils/package.jsonpackages/kg-utils/src/index.tspackages/kg-utils/src/slugify.tspackages/kg-utils/test/slugify.test.tspackages/kg-utils/test/utils/assertions.jspackages/kg-utils/test/utils/assertions.tspackages/kg-utils/test/utils/index.jspackages/kg-utils/test/utils/index.tspackages/kg-utils/test/utils/overrides.jspackages/kg-utils/test/utils/overrides.tspackages/kg-utils/tsconfig.json
💤 Files with no reviewable changes (5)
- packages/kg-utils/test/utils/index.js
- packages/kg-utils/test/utils/overrides.js
- packages/kg-utils/lib/kg-utils.js
- packages/kg-utils/index.js
- packages/kg-utils/test/utils/assertions.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/kg-utils/src/index.ts
- packages/kg-utils/tsconfig.json
- packages/kg-utils/test/utils/index.ts
- packages/kg-utils/.gitignore
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/kg-utils/package.json (1)
11-13: Reduce script drift by reusing thebuildscript.Lines 12-13 duplicate Line 11. Reusing
buildkeeps lifecycle scripts consistent when build options evolve.♻️ Suggested update
- "build": "tsc", - "prepare": "tsc", - "pretest": "tsc", + "build": "tsc", + "prepare": "yarn build", + "pretest": "yarn build",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-utils/package.json` around lines 11 - 13, The package.json lifecycle scripts duplicate the same tsc command; update "prepare" and "pretest" to reuse the single "build" script instead of repeating the command. Change the "prepare" and "pretest" script values to invoke the build script (e.g., "npm run build" or "yarn build") so that "build" remains the single source of truth for compilation and future build option changes only need to be made in the "build" script.
🤖 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/kg-utils/package.json`:
- Line 14: Update the package.json "test" script to stop using the CommonJS
loader flag and instead rely on Node's import hook by removing the "--require
tsx" from the mocha invocation; also add or update your Mocha config
(.mocharc.json) to include the node-option ["import=tsx"] so tsx is loaded via
Node's --import mechanism for proper ESM support (target the "test" script entry
in package.json and the Mocha config file for these changes).
---
Nitpick comments:
In `@packages/kg-utils/package.json`:
- Around line 11-13: The package.json lifecycle scripts duplicate the same tsc
command; update "prepare" and "pretest" to reuse the single "build" script
instead of repeating the command. Change the "prepare" and "pretest" script
values to invoke the build script (e.g., "npm run build" or "yarn build") so
that "build" remains the single source of truth for compilation and future build
option changes only need to be made in the "build" script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f82f2b6-1cae-4cf6-8bf9-5b415b177bc2
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
packages/kg-utils/package.json
kevinansfield
force-pushed
the
ts/kg-utils
branch
from
88b94c0 to
6398233
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/kg-utils/test/slugify.test.ts (1)
1-2: Add at least one test through the exported entry point.Importing
../src/slugify.jskeeps this suite on the implementation path, so the new export wiring can regress without a failing test. Switching this import to../src/index.js, or adding a small smoke test through it, would cover the migration surface too.♻️ Suggested change
-import slugify from '../src/slugify.js'; +import slugify from '../src/index.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-utils/test/slugify.test.ts` around lines 1 - 2, The test currently imports the implementation directly (slugify from ../src/slugify.js) so add coverage through the package entry point by switching the test to import slugify from ../src/index.js or adding a small smoke test that imports the entry module and calls slugify('Foo Bar') expecting 'foo-bar'; update the existing test (slugify.test.ts) to use the exported symbol slugify from the package entry point (index.js) so any export wiring regressions will be caught.packages/kg-utils/src/slugify.ts (1)
3-6: Constraintypeto the supported modes.At lines 15-26, the function only handles
type === 'markdown'or defaults to mobiledoc. KeepingSlugifyOptions.typeasstringallows typos to silently compile and fall through to mobiledoc instead of failing at the type level.♻️ Suggested change
interface SlugifyOptions { ghostVersion?: string; - type?: string; + type?: 'mobiledoc' | 'markdown'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-utils/src/slugify.ts` around lines 3 - 6, The SlugifyOptions.type should be constrained to the supported modes to catch typos at compile time: change the interface SlugifyOptions so the type field is a string union (e.g., type?: 'markdown' | 'mobiledoc') and update any related signatures/usages (the function that branches on type === 'markdown' / default mobiledoc) to rely on this union so the compiler enforces valid values and enables exhaustive checks; adjust any call sites to use one of the union members if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/kg-utils/src/slugify.ts`:
- Around line 3-6: The SlugifyOptions.type should be constrained to the
supported modes to catch typos at compile time: change the interface
SlugifyOptions so the type field is a string union (e.g., type?: 'markdown' |
'mobiledoc') and update any related signatures/usages (the function that
branches on type === 'markdown' / default mobiledoc) to rely on this union so
the compiler enforces valid values and enables exhaustive checks; adjust any
call sites to use one of the union members if necessary.
In `@packages/kg-utils/test/slugify.test.ts`:
- Around line 1-2: The test currently imports the implementation directly
(slugify from ../src/slugify.js) so add coverage through the package entry point
by switching the test to import slugify from ../src/index.js or adding a small
smoke test that imports the entry module and calls slugify('Foo Bar') expecting
'foo-bar'; update the existing test (slugify.test.ts) to use the exported symbol
slugify from the package entry point (index.js) so any export wiring regressions
will be caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56a38eb8-a1ad-4e17-9eb5-cb2aa0dbbb45
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
packages/kg-utils/.gitignorepackages/kg-utils/eslint.config.mjspackages/kg-utils/index.jspackages/kg-utils/lib/kg-utils.jspackages/kg-utils/package.jsonpackages/kg-utils/src/index.tspackages/kg-utils/src/slugify.tspackages/kg-utils/test/slugify.test.tspackages/kg-utils/test/utils/assertions.jspackages/kg-utils/test/utils/assertions.tspackages/kg-utils/test/utils/index.jspackages/kg-utils/test/utils/index.tspackages/kg-utils/test/utils/overrides.jspackages/kg-utils/test/utils/overrides.tspackages/kg-utils/tsconfig.json
💤 Files with no reviewable changes (5)
- packages/kg-utils/index.js
- packages/kg-utils/test/utils/assertions.js
- packages/kg-utils/lib/kg-utils.js
- packages/kg-utils/test/utils/index.js
- packages/kg-utils/test/utils/overrides.js
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/kg-utils/test/utils/index.ts
- packages/kg-utils/test/utils/assertions.ts
- packages/kg-utils/package.json
- packages/kg-utils/.gitignore
- packages/kg-utils/eslint.config.mjs
- packages/kg-utils/test/utils/overrides.ts
- packages/kg-utils/tsconfig.json
- packages/kg-utils/src/index.ts
kevinansfield
force-pushed
the
ts/kg-utils
branch
from
6398233 to
a746087
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/kg-utils/package.json (1)
11-13: Reduce duplicated compile commands across lifecycle scripts.Line 11, Line 12, and Line 13 all execute
tsc. Consider routingprepare/pretestthroughbuildto keep one source of truth.♻️ Suggested script deduplication
- "build": "tsc", - "prepare": "tsc", - "pretest": "tsc", + "build": "tsc", + "prepare": "yarn build", + "pretest": "yarn build",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-utils/package.json` around lines 11 - 13, The package.json currently runs "tsc" directly in three lifecycle scripts ("build", "prepare", "pretest"); change "prepare" and "pretest" to invoke the single source-of-truth "build" script instead (e.g., set "prepare" and "pretest" to run the "build" script via npm run), keeping "build" as "tsc" so compilation logic is centralized under the "build" script.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/kg-utils/package.json`:
- Around line 11-13: The package.json currently runs "tsc" directly in three
lifecycle scripts ("build", "prepare", "pretest"); change "prepare" and
"pretest" to invoke the single source-of-truth "build" script instead (e.g., set
"prepare" and "pretest" to run the "build" script via npm run), keeping "build"
as "tsc" so compilation logic is centralized under the "build" script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ace81999-e7dd-4570-b2eb-f556f52dfa4b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
packages/kg-utils/.gitignorepackages/kg-utils/eslint.config.mjspackages/kg-utils/index.jspackages/kg-utils/lib/kg-utils.jspackages/kg-utils/package.jsonpackages/kg-utils/src/index.tspackages/kg-utils/src/slugify.tspackages/kg-utils/test/slugify.test.tspackages/kg-utils/test/utils/assertions.tspackages/kg-utils/test/utils/index.tspackages/kg-utils/test/utils/overrides.jspackages/kg-utils/test/utils/overrides.tspackages/kg-utils/tsconfig.json
💤 Files with no reviewable changes (3)
- packages/kg-utils/index.js
- packages/kg-utils/lib/kg-utils.js
- packages/kg-utils/test/utils/overrides.js
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/kg-utils/test/utils/index.ts
- packages/kg-utils/test/slugify.test.ts
- packages/kg-utils/tsconfig.json
- packages/kg-utils/eslint.config.mjs
- packages/kg-utils/.gitignore
kevinansfield
force-pushed
the
ts/kg-utils
branch
from
a746087 to
c1dabfb
Compare
- Move lib/ to src/, rename .js to .ts - Add tsconfig.json (strict, NodeNext, ESM) - Add "type": "module" to package.json - Convert source and tests from CJS to ESM - Add type annotations throughout - Replace .eslintrc.js with eslint.config.js (flat config) - Output to build/ via tsc
kevinansfield
force-pushed
the
ts/kg-utils
branch
from
c1dabfb to
1cbb44f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
yarn testpasses in kg-utilsyarn lintpasses in kg-utils