Open
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:
WalkthroughThe PR migrates packages/kg-card-factory from CommonJS JavaScript to a TypeScript + ESM package. Changes include new TypeScript sources (src/CardFactory.ts, src/index.ts), tsconfig and cjs tsconfig, updated package.json (build outputs, exports, scripts, devDependencies), ESLint config migration, addition of build artifacts and .gitignore entries, replacement of CommonJS root export, and conversion of tests and test utilities to TypeScript/ESM with related deletions and additions. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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)
📝 Coding Plan
Comment |
kevinansfield
force-pushed
the
ts/kg-card-factory
branch
from
06bd64b to
743f091
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/kg-card-factory/src/CardFactory.ts (3)
69-69: Type assertion is acceptable but could be cleaner.The cast
as {appendChild(child: unknown): void}works but relies on the interface returningunknownfromcreateDocumentFragment. If you expandCardRenderEnvto include proper DOM types (or use a more complete DOM library type), this assertion could be eliminated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-card-factory/src/CardFactory.ts` at line 69, The fragment variable is being type-asserted with a minimal shape; instead update the CardRenderEnv DOM typing (or import proper lib.dom types) so dom.createDocumentFragment() returns the correct DocumentFragment type and remove the ad-hoc cast on fragment. Locate the call to dom.createDocumentFragment() in CardFactory (symbol: fragment) and either extend CardRenderEnv to include standard DOM interfaces or augment the dom type so the returned value already has appendChild and other DocumentFragment members, then delete the inline "as {appendChild(child: unknown): void}" assertion.
16-22:CardRenderEnvinterface is incomplete for actual card implementations.The interface only declares
createCommentandcreateDocumentFragment, but real card implementations (as seen in the test file and kg-default-cards context) use additional DOM methods likecreateElementandcreateTextNode. This will cause type errors for consumers implementing theCardDefinition.rendermethod.Proposed expansion of the interface
interface CardRenderEnv { dom: { createComment(text: string): unknown; createDocumentFragment(): unknown; + createElement(tagName: string): unknown; + createTextNode(text: string): unknown; }; [key: string]: unknown; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-card-factory/src/CardFactory.ts` around lines 16 - 22, The CardRenderEnv interface is missing DOM methods used by real cards and will cause type errors in CardDefinition.render implementations; update the CardRenderEnv type (the dom property) to include createElement(tagName: string): unknown, createTextNode(text: string): unknown and any other commonly used DOM factory methods (e.g., createElementNS, createComment, createDocumentFragment) so consumers can implement CardDefinition.render without type errors; ensure the updated interface name CardRenderEnv and the dom property are the ones extended so existing references remain valid.
79-104: Consider extracting repeateddefaultOptionscreation.The same default options object
{assetsOnly: false, siteUrl: factoryOptions.siteUrl}is created in three places. While this works, extracting it could reduce duplication.Optional refactor to DRY up defaultOptions
createCard(card: CardDefinition) { const {factoryOptions} = this; const {name, type, config = {}} = card; + const getDefaultOptions = () => ({assetsOnly: false, siteUrl: factoryOptions.siteUrl}); return { // ... render method unchanged ... absoluteToRelative(payload: CardPayload, _options?: CardTransformOptions) { if (card.absoluteToRelative) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; + const defaultOptions = getDefaultOptions(); const options = Object.assign({}, defaultOptions, _options); return card.absoluteToRelative(payload, options); } return payload; }, relativeToAbsolute(payload: CardPayload, _options?: CardTransformOptions) { if (card.relativeToAbsolute) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; + const defaultOptions = getDefaultOptions(); const options = Object.assign({}, defaultOptions, _options); return card.relativeToAbsolute(payload, options); } return payload; }, toTransformReady(payload: CardPayload, _options?: CardTransformOptions) { if (card.toTransformReady) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; + const defaultOptions = getDefaultOptions(); const options = Object.assign({}, defaultOptions, _options); return card.toTransformReady(payload, options); } return payload; } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-card-factory/src/CardFactory.ts` around lines 79 - 104, Create a single reusable default options object or helper to avoid repeating {assetsOnly: false, siteUrl: factoryOptions.siteUrl} in absoluteToRelative, relativeToAbsolute, and toTransformReady: e.g., define const defaultTransformOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl} (or a getDefaultOptions(_options) that merges with _options) and use Object.assign({}, defaultTransformOptions, _options) in each of the three methods (absoluteToRelative, relativeToAbsolute, toTransformReady) so they all reuse the same default configuration.
🤖 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-card-factory/src/index.ts`:
- Line 1: The current re-export `export {default} from './CardFactory.js'`
changes the CommonJS shape (it becomes `exports.default = ...`) so
require('@tryghost/kg-card-factory') returns an object with .default; replace
the ES-style re-export with a CommonJS-compatible export so requiring the
package returns CardFactory directly—for example, in
packages/kg-card-factory/src/index.ts import or require './CardFactory.js' and
assign the export to module.exports (e.g., const CardFactory =
require('./CardFactory.js'); module.exports = CardFactory;), ensuring consumers
using require() get the CardFactory value rather than an object with a .default
property.
In `@packages/kg-card-factory/test/CardFactory.test.ts`:
- Line 82: The test calls card.relativeToAbsolute(undefined) causing a type
mismatch; update the test to pass a valid string (e.g.,
card.relativeToAbsolute('') or the expected base path) instead of undefined, or
alternatively update the relativeToAbsolute method signature to accept undefined
(e.g., relativeToAbsolute(base?: string)) and handle undefined inside the
method; prefer changing the test to use an explicit empty string or proper base
URL to keep the method signature unchanged.
- Line 63: The test calls card.absoluteToRelative(undefined) but
absoluteToRelative(payload: CardPayload, _options?: CardTransformOptions)
requires a CardPayload; update the test to pass a valid payload (e.g., an empty
object conforming to CardPayload) or modify the absoluteToRelative signature to
accept payload?: CardPayload so it can handle undefined—adjust the
implementation in absoluteToRelative to treat undefined as an empty payload if
you change the signature.
In `@packages/kg-card-factory/tsconfig.json`:
- Line 6: The tests are excluded from the TypeScript program because
tsconfig.json sets "rootDir": "src" and include only covers src/**/*; create a
companion tsconfig for tests (e.g., tsconfig.test.json) that extends the
existing tsconfig.json, sets "noEmit": true and "rootDir": "." and adds include
entries for both "src/**/*" and "test/**/*" so the test/*.ts files are
type-checked without being emitted; update CI/pretest to run tsc -p
tsconfig.test.json or point your pretest script to use that config.
---
Nitpick comments:
In `@packages/kg-card-factory/src/CardFactory.ts`:
- Line 69: The fragment variable is being type-asserted with a minimal shape;
instead update the CardRenderEnv DOM typing (or import proper lib.dom types) so
dom.createDocumentFragment() returns the correct DocumentFragment type and
remove the ad-hoc cast on fragment. Locate the call to
dom.createDocumentFragment() in CardFactory (symbol: fragment) and either extend
CardRenderEnv to include standard DOM interfaces or augment the dom type so the
returned value already has appendChild and other DocumentFragment members, then
delete the inline "as {appendChild(child: unknown): void}" assertion.
- Around line 16-22: The CardRenderEnv interface is missing DOM methods used by
real cards and will cause type errors in CardDefinition.render implementations;
update the CardRenderEnv type (the dom property) to include
createElement(tagName: string): unknown, createTextNode(text: string): unknown
and any other commonly used DOM factory methods (e.g., createElementNS,
createComment, createDocumentFragment) so consumers can implement
CardDefinition.render without type errors; ensure the updated interface name
CardRenderEnv and the dom property are the ones extended so existing references
remain valid.
- Around line 79-104: Create a single reusable default options object or helper
to avoid repeating {assetsOnly: false, siteUrl: factoryOptions.siteUrl} in
absoluteToRelative, relativeToAbsolute, and toTransformReady: e.g., define const
defaultTransformOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}
(or a getDefaultOptions(_options) that merges with _options) and use
Object.assign({}, defaultTransformOptions, _options) in each of the three
methods (absoluteToRelative, relativeToAbsolute, toTransformReady) so they all
reuse the same default configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5bc7b98-b28c-4b4d-bf88-7b38f9a96153
📒 Files selected for processing (15)
packages/kg-card-factory/.gitignorepackages/kg-card-factory/eslint.config.mjspackages/kg-card-factory/index.jspackages/kg-card-factory/package.jsonpackages/kg-card-factory/src/CardFactory.tspackages/kg-card-factory/src/index.tspackages/kg-card-factory/test/CardFactory.test.jspackages/kg-card-factory/test/CardFactory.test.tspackages/kg-card-factory/test/utils/assertions.jspackages/kg-card-factory/test/utils/assertions.tspackages/kg-card-factory/test/utils/index.jspackages/kg-card-factory/test/utils/index.tspackages/kg-card-factory/test/utils/overrides.jspackages/kg-card-factory/test/utils/overrides.tspackages/kg-card-factory/tsconfig.json
💤 Files with no reviewable changes (5)
- packages/kg-card-factory/test/CardFactory.test.js
- packages/kg-card-factory/test/utils/index.js
- packages/kg-card-factory/test/utils/overrides.js
- packages/kg-card-factory/index.js
- packages/kg-card-factory/test/utils/assertions.js
| @@ -0,0 +1 @@ | |||
| export {default} from './CardFactory.js'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TypeScript, when compilerOptions.moduleisCommonJS, how does export { default } from "./CardFactory.js"compile, and doesrequire("./build/index.js")return the default export directly or an object with a.default property?
💡 Result:
With compilerOptions.module: "CommonJS", TypeScript keeps the “default export” as a property on exports, and a re-export like:
export { default } from "./CardFactory.js";compiles to CommonJS roughly like:
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
var CardFactory_js_1 = require("./CardFactory.js");
Object.defineProperty(exports, "default", {
enumerable: true,
get: function () { return CardFactory_js_1.default; }
});So:
require("./build/index.js")returns the exports object, not the default value directly.- The re-exported default is at
require("./build/index.js").default.
This is the standard interop shape TypeScript emits forexport defaultwhen targeting CommonJS. [1][2][3]
Sources:
[1] https://www.typescriptlang.org/docs/handbook/modules/appendices/esm-cjs-interop.html
[2] https://blog.andrewbran.ch/default-exports-in-commonjs-libraries/
[3] https://stackoverflow.com/questions/60577733/not-able-to-require-default-exported-function-from-typescript-file-in-nodejs
🏁 Script executed:
# Check if the kg-card-factory package exists and inspect its configuration
if [ -d "packages/kg-card-factory" ]; then
echo "=== Package exists ==="
echo "=== tsconfig.json ==="
cat packages/kg-card-factory/tsconfig.json 2>/dev/null || echo "File not found"
echo ""
echo "=== package.json (main field) ==="
cat packages/kg-card-factory/package.json 2>/dev/null | grep -A 5 -B 5 '"main"' || echo "Not found"
echo ""
echo "=== index.ts content ==="
cat packages/kg-card-factory/src/index.ts 2>/dev/null || echo "File not found"
else
echo "Package directory does not exist"
fiRepository: TryGhost/Koenig
Length of output: 1030
🏁 Script executed:
# Search for imports of kg-card-factory in the codebase to identify consumers
echo "=== Searching for imports of kg-card-factory ==="
rg "from ['\"]@tryghost/kg-card-factory|require\(['\"]@tryghost/kg-card-factory" --type ts --type js -nRepository: TryGhost/Koenig
Length of output: 107
This changes the CommonJS entry shape.
With compilerOptions.module set to CommonJS, the re-export export {default} from './CardFactory.js' compiles to exports.default = ..., so require('@tryghost/kg-card-factory') returns an object with a .default property instead of returning CardFactory directly. This is a breaking change for existing CommonJS consumers unless the package is intentionally switching to true ESM.
Possible compatibility-preserving fix
-export {default} from './CardFactory.js';
+import CardFactory from './CardFactory.js';
+
+export = CardFactory;📝 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
| export {default} from './CardFactory.js'; | |
| import CardFactory from './CardFactory.js'; | |
| export = CardFactory; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-card-factory/src/index.ts` at line 1, The current re-export
`export {default} from './CardFactory.js'` changes the CommonJS shape (it
becomes `exports.default = ...`) so require('@tryghost/kg-card-factory') returns
an object with .default; replace the ES-style re-export with a
CommonJS-compatible export so requiring the package returns CardFactory
directly—for example, in packages/kg-card-factory/src/index.ts import or require
'./CardFactory.js' and assign the export to module.exports (e.g., const
CardFactory = require('./CardFactory.js'); module.exports = CardFactory;),
ensuring consumers using require() get the CardFactory value rather than an
object with a .default property.
Member
Author
There was a problem hiding this comment.
esModuleInterop handles this for TS consumers, and the existing JS code already used export default so isn't this the same behaviour as before?
There was a problem hiding this comment.
@kevinansfield, you raise a fair point — esModuleInterop does handle this correctly for TypeScript consumers, allowing import CardFactory from '@tryghost/kg-card-factory' to work seamlessly even when the CJS output exposes exports.default.
However, there's a subtle distinction worth noting: the old index.js actually used module.exports = require('./lib/CardFactory') (not export default), so the old CJS shape returned CardFactory directly from require(...), while the new CJS build of export {default} would produce exports.default = CardFactory. That said, if all consumers in the monorepo are TypeScript projects with esModuleInterop: true (which the search results suggest is the case), this shape difference has no practical impact — your existing consumers would be unaffected.
So my original comment was overstated for your specific use case. You're right that for TypeScript consumers, this is effectively equivalent behavior.
✏️ Learnings added
Learnt from: kevinansfield
Repo: TryGhost/Koenig PR: 1758
File: packages/kg-card-factory/src/index.ts:1-1
Timestamp: 2026-03-17T22:14:21.295Z
Learning: In the TryGhost/Koenig monorepo, all consumers of `tryghost/kg-card-factory` are TypeScript packages with `esModuleInterop: true`. The package uses ESM-style `export {default}` in `src/index.ts`, which is fine for these consumers even when compiled to CJS. Raw CommonJS `require()` callers are not a concern for this package.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
kevinansfield
force-pushed
the
ts/kg-card-factory
branch
from
743f091 to
6dde870
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/kg-card-factory/test/CardFactory.test.ts (1)
63-63:⚠️ Potential issue | 🟡 MinorPass a typed payload value in transform tests (or intentionally relax the API type).
Line 63 and Line 82 pass
undefined, but the current transform signatures expectCardPayload. This is the same type-mismatch concern noted earlier.Suggested diff
- card.absoluteToRelative(undefined).should.equal('http://127.0.0.1:2368/'); + card.absoluteToRelative({}).should.equal('http://127.0.0.1:2368/'); ... - card.relativeToAbsolute(undefined).should.equal('http://127.0.0.1:2368/'); + card.relativeToAbsolute({}).should.equal('http://127.0.0.1:2368/');#!/bin/bash # Verify signature vs call-site mismatch context (read-only) fd tsconfig.json packages/kg-card-factory --exec cat rg -n -C2 "absoluteToRelative\(undefined\)|relativeToAbsolute\(undefined\)" packages/kg-card-factory/test/CardFactory.test.ts rg -n -C2 "absoluteToRelative\(payload: CardPayload|relativeToAbsolute\(payload: CardPayload" packages/kg-card-factory/src/CardFactory.tsAlso applies to: 82-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-card-factory/test/CardFactory.test.ts` at line 63, Tests call card.absoluteToRelative(undefined) and card.relativeToAbsolute(undefined) but the transform signatures require CardPayload; either update the tests to pass a properly typed CardPayload fixture (e.g., a minimal payload object used by other tests) to absoluteToRelative and relativeToAbsolute, or change the method signatures in CardFactory (absoluteToRelative / relativeToAbsolute) to accept CardPayload | undefined (and handle undefined internally). Locate the methods absoluteToRelative and relativeToAbsolute in CardFactory and the two test calls in CardFactory.test.ts (lines around the existing calls) and make the change consistently so the types align.
🧹 Nitpick comments (2)
packages/kg-card-factory/package.json (1)
11-13: De-duplicate compile script wiring.
build,prepare, andpretestall runtsc. Pointprepare/pretesttoyarn buildto prevent command drift.Suggested diff
- "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-card-factory/package.json` around lines 11 - 13, The package.json currently has three scripts ("build", "prepare", "pretest") all invoking "tsc" directly; update "prepare" and "pretest" to call the canonical build script instead (e.g., "yarn build" or "npm run build") so they reuse the "build" entry and avoid duplicate command strings—modify the "prepare" and "pretest" script values to reference the build script while leaving "build" as "tsc".packages/kg-card-factory/src/CardFactory.ts (1)
79-103: Extract shared transform option merge logic.Line 81, Line 90, and Line 99 repeat the same default-options composition. A small helper reduces duplication and drift risk.
Suggested diff
export default class CardFactory { factoryOptions: FactoryOptions; @@ createCard(card: CardDefinition) { const {factoryOptions} = this; const {name, type, config = {}} = card; + const getTransformOptions = (_options?: CardTransformOptions) => { + const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; + return Object.assign({}, defaultOptions, _options); + }; return { @@ absoluteToRelative(payload: CardPayload, _options?: CardTransformOptions) { if (card.absoluteToRelative) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; - const options = Object.assign({}, defaultOptions, _options); + const options = getTransformOptions(_options); return card.absoluteToRelative(payload, options); } return payload; }, relativeToAbsolute(payload: CardPayload, _options?: CardTransformOptions) { if (card.relativeToAbsolute) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; - const options = Object.assign({}, defaultOptions, _options); + const options = getTransformOptions(_options); return card.relativeToAbsolute(payload, options); } return payload; }, toTransformReady(payload: CardPayload, _options?: CardTransformOptions) { if (card.toTransformReady) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; - const options = Object.assign({}, defaultOptions, _options); + const options = getTransformOptions(_options); return card.toTransformReady(payload, options); } return payload; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-card-factory/src/CardFactory.ts` around lines 79 - 103, Extract the repeated default-options composition into a small helper (e.g., getTransformOptions or mergeTransformOptions) that returns Object.assign({}, {assetsOnly: false, siteUrl: factoryOptions.siteUrl}, _options) and use it in absoluteToRelative, relativeToAbsolute, and toTransformReady instead of duplicating the defaultOptions/options block; update calls to pass the returned options to card.absoluteToRelative, card.relativeToAbsolute, and card.toTransformReady respectively to keep behavior identical while removing duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/kg-card-factory/test/CardFactory.test.ts`:
- Line 63: Tests call card.absoluteToRelative(undefined) and
card.relativeToAbsolute(undefined) but the transform signatures require
CardPayload; either update the tests to pass a properly typed CardPayload
fixture (e.g., a minimal payload object used by other tests) to
absoluteToRelative and relativeToAbsolute, or change the method signatures in
CardFactory (absoluteToRelative / relativeToAbsolute) to accept CardPayload |
undefined (and handle undefined internally). Locate the methods
absoluteToRelative and relativeToAbsolute in CardFactory and the two test calls
in CardFactory.test.ts (lines around the existing calls) and make the change
consistently so the types align.
---
Nitpick comments:
In `@packages/kg-card-factory/package.json`:
- Around line 11-13: The package.json currently has three scripts ("build",
"prepare", "pretest") all invoking "tsc" directly; update "prepare" and
"pretest" to call the canonical build script instead (e.g., "yarn build" or "npm
run build") so they reuse the "build" entry and avoid duplicate command
strings—modify the "prepare" and "pretest" script values to reference the build
script while leaving "build" as "tsc".
In `@packages/kg-card-factory/src/CardFactory.ts`:
- Around line 79-103: Extract the repeated default-options composition into a
small helper (e.g., getTransformOptions or mergeTransformOptions) that returns
Object.assign({}, {assetsOnly: false, siteUrl: factoryOptions.siteUrl},
_options) and use it in absoluteToRelative, relativeToAbsolute, and
toTransformReady instead of duplicating the defaultOptions/options block; update
calls to pass the returned options to card.absoluteToRelative,
card.relativeToAbsolute, and card.toTransformReady respectively to keep behavior
identical while removing duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4686e8a9-3e9f-43c6-aa9e-a8a1dbb45c17
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
packages/kg-card-factory/.gitignorepackages/kg-card-factory/eslint.config.mjspackages/kg-card-factory/index.jspackages/kg-card-factory/package.jsonpackages/kg-card-factory/src/CardFactory.tspackages/kg-card-factory/src/index.tspackages/kg-card-factory/test/CardFactory.test.jspackages/kg-card-factory/test/CardFactory.test.tspackages/kg-card-factory/test/utils/assertions.jspackages/kg-card-factory/test/utils/assertions.tspackages/kg-card-factory/test/utils/index.jspackages/kg-card-factory/test/utils/index.tspackages/kg-card-factory/test/utils/overrides.jspackages/kg-card-factory/test/utils/overrides.tspackages/kg-card-factory/tsconfig.json
💤 Files with no reviewable changes (5)
- packages/kg-card-factory/test/utils/assertions.js
- packages/kg-card-factory/test/utils/overrides.js
- packages/kg-card-factory/test/utils/index.js
- packages/kg-card-factory/index.js
- packages/kg-card-factory/test/CardFactory.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/kg-card-factory/src/index.ts
- packages/kg-card-factory/tsconfig.json
- packages/kg-card-factory/test/utils/assertions.ts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1758 +/- ##
==========================================
+ Coverage 88.51% 88.67% +0.15%
==========================================
Files 201 200 -1
Lines 18456 18466 +10
Branches 2019 2023 +4
==========================================
+ Hits 16337 16375 +38
+ Misses 2108 2080 -28
Partials 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kevinansfield
force-pushed
the
ts/kg-card-factory
branch
from
6dde870 to
7716d5a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/kg-card-factory/src/CardFactory.ts (1)
79-103: Deduplicate transform option merging to prevent drift.The three transform methods repeat identical default-option assembly. Extracting one local helper improves maintainability and reduces future divergence risk.
♻️ Proposed refactor
return { name, type, factoryOptions, + _mergeTransformOptions(_options?: CardTransformOptions) { + const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; + return Object.assign({}, defaultOptions, _options); + }, render({env, payload, options}: CardRenderArgs) { const {dom} = env; const cardOptions = Object.assign({}, factoryOptions, options); const cardOutput = card.render({env, payload, options: cardOptions}); @@ absoluteToRelative(payload: CardPayload, _options?: CardTransformOptions) { if (card.absoluteToRelative) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; - const options = Object.assign({}, defaultOptions, _options); + const options = this._mergeTransformOptions(_options); return card.absoluteToRelative(payload, options); } return payload; }, relativeToAbsolute(payload: CardPayload, _options?: CardTransformOptions) { if (card.relativeToAbsolute) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; - const options = Object.assign({}, defaultOptions, _options); + const options = this._mergeTransformOptions(_options); return card.relativeToAbsolute(payload, options); } return payload; }, toTransformReady(payload: CardPayload, _options?: CardTransformOptions) { if (card.toTransformReady) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; - const options = Object.assign({}, defaultOptions, _options); + const options = this._mergeTransformOptions(_options); return card.toTransformReady(payload, options); } return payload; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-card-factory/src/CardFactory.ts` around lines 79 - 103, The three transform methods absoluteToRelative, relativeToAbsolute, and toTransformReady repeat identical default-options assembly; create a small local helper (e.g., mergeTransformOptions or getTransformOptions) inside CardFactory that accepts _options and returns Object.assign({}, {assetsOnly: false, siteUrl: factoryOptions.siteUrl}, _options), then replace the duplicated defaultOptions/options construction in each of those three methods with a call to that helper and pass the resulting options into card.absoluteToRelative/card.relativeToAbsolute/card.toTransformReady; ensure behavior remains identical when card.* is absent.packages/kg-card-factory/package.json (1)
11-14: Remove redundant compilation in pretest; use tsx for direct TypeScript execution.The test script already uses
--require tsxto execute.test.tsfiles directly, sopretest: tscis unnecessary and slows CI. No references to compiled output exist in the tests.Suggested refactor
"scripts": { "dev": "echo \"Implement me!\"", "build": "tsc", - "prepare": "tsc", - "pretest": "tsc", + "prepare": "yarn build", + "typecheck": "tsc --noEmit", + "pretest": "yarn typecheck", "test": "NODE_ENV=testing c8 --all --reporter text --reporter cobertura mocha --require tsx './test/**/*.test.ts'", "lint": "eslint . --cache", "posttest": "yarn lint" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-card-factory/package.json` around lines 11 - 14, The package.json currently runs TypeScript compilation twice because "pretest": "tsc" is redundant given the "test" script already runs tests via tsx (test uses "--require tsx"); remove the "pretest" script entry so tests run directly without a prior tsc compile, keeping "build": "tsc" and "prepare": "tsc" intact; update the scripts section to delete the pretest key (or set it to a no-op) and verify "test" still contains NODE_ENV=testing c8 --all --reporter text --reporter cobertura mocha --require tsx './test/**/*.test.ts'.
🤖 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-card-factory/package.json`:
- Around line 11-14: The package.json currently runs TypeScript compilation
twice because "pretest": "tsc" is redundant given the "test" script already runs
tests via tsx (test uses "--require tsx"); remove the "pretest" script entry so
tests run directly without a prior tsc compile, keeping "build": "tsc" and
"prepare": "tsc" intact; update the scripts section to delete the pretest key
(or set it to a no-op) and verify "test" still contains NODE_ENV=testing c8
--all --reporter text --reporter cobertura mocha --require tsx
'./test/**/*.test.ts'.
In `@packages/kg-card-factory/src/CardFactory.ts`:
- Around line 79-103: The three transform methods absoluteToRelative,
relativeToAbsolute, and toTransformReady repeat identical default-options
assembly; create a small local helper (e.g., mergeTransformOptions or
getTransformOptions) inside CardFactory that accepts _options and returns
Object.assign({}, {assetsOnly: false, siteUrl: factoryOptions.siteUrl},
_options), then replace the duplicated defaultOptions/options construction in
each of those three methods with a call to that helper and pass the resulting
options into
card.absoluteToRelative/card.relativeToAbsolute/card.toTransformReady; ensure
behavior remains identical when card.* is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84777bfa-f4b3-4633-baad-e0e46e7d873a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
packages/kg-card-factory/.gitignorepackages/kg-card-factory/eslint.config.mjspackages/kg-card-factory/index.jspackages/kg-card-factory/package.jsonpackages/kg-card-factory/src/CardFactory.tspackages/kg-card-factory/src/index.tspackages/kg-card-factory/test/CardFactory.test.jspackages/kg-card-factory/test/CardFactory.test.tspackages/kg-card-factory/test/utils/assertions.jspackages/kg-card-factory/test/utils/assertions.tspackages/kg-card-factory/test/utils/index.jspackages/kg-card-factory/test/utils/index.tspackages/kg-card-factory/test/utils/overrides.jspackages/kg-card-factory/test/utils/overrides.tspackages/kg-card-factory/tsconfig.json
💤 Files with no reviewable changes (5)
- packages/kg-card-factory/test/utils/assertions.js
- packages/kg-card-factory/test/CardFactory.test.js
- packages/kg-card-factory/test/utils/overrides.js
- packages/kg-card-factory/test/utils/index.js
- packages/kg-card-factory/index.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/kg-card-factory/test/CardFactory.test.ts
- packages/kg-card-factory/test/utils/overrides.ts
- packages/kg-card-factory/.gitignore
- packages/kg-card-factory/tsconfig.json
kevinansfield
force-pushed
the
ts/kg-card-factory
branch
from
7716d5a to
3bf164d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kg-card-factory/src/CardFactory.ts (1)
61-63:⚠️ Potential issue | 🟡 MinorAdd explicit test coverage for the empty-text early return path.
Line 61 changes control flow and bypasses wrapper logic, so this branch should be pinned with a unit test to prevent regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-card-factory/src/CardFactory.ts` around lines 61 - 63, Add a unit test that exercises the new early-return branch where cardOutput.nodeType === 3 and cardOutput.nodeValue === '' so regressions are caught: construct a minimal text-node-like object with nodeType 3 and nodeValue '' and pass it through the CardFactory method that produces/returns cardOutput (the code in CardFactory.ts that contains the early return), assert that the exact object is returned and that wrapper/transform logic is not invoked; mock or spy any wrapper functions to ensure they are not called.
🧹 Nitpick comments (1)
packages/kg-card-factory/src/CardFactory.ts (1)
79-103: Deduplicate transform option merging across the three transformer methods.The repeated
defaultOptions+Object.assignblock is a good candidate for a small helper.♻️ Proposed refactor
export default class CardFactory { factoryOptions: FactoryOptions; @@ createCard(card: CardDefinition) { const {factoryOptions} = this; const {name, type, config = {}} = card; + const buildTransformOptions = (_options?: CardTransformOptions): CardTransformOptions => { + return Object.assign( + {assetsOnly: false, siteUrl: factoryOptions.siteUrl}, + _options + ); + }; @@ absoluteToRelative(payload: CardPayload, _options?: CardTransformOptions) { if (card.absoluteToRelative) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; - const options = Object.assign({}, defaultOptions, _options); + const options = buildTransformOptions(_options); return card.absoluteToRelative(payload, options); } return payload; }, @@ relativeToAbsolute(payload: CardPayload, _options?: CardTransformOptions) { if (card.relativeToAbsolute) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; - const options = Object.assign({}, defaultOptions, _options); + const options = buildTransformOptions(_options); return card.relativeToAbsolute(payload, options); } return payload; }, @@ toTransformReady(payload: CardPayload, _options?: CardTransformOptions) { if (card.toTransformReady) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; - const options = Object.assign({}, defaultOptions, _options); + const options = buildTransformOptions(_options); return card.toTransformReady(payload, options); } return payload; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-card-factory/src/CardFactory.ts` around lines 79 - 103, The three transformer methods absoluteToRelative, relativeToAbsolute, and toTransformReady repeat the same defaultOptions + Object.assign merging logic; extract that into a small helper (e.g., buildTransformOptions or mergeTransformOptions) that accepts the incoming _options and returns Object.assign({}, {assetsOnly: false, siteUrl: factoryOptions.siteUrl}, _options), then replace the repeated blocks in absoluteToRelative, relativeToAbsolute, and toTransformReady to call the helper and pass the resulting options into card.absoluteToRelative, card.relativeToAbsolute, and card.toTransformReady respectively; ensure the helper is defined alongside the CardFactory code so it can reference factoryOptions.siteUrl and the CardTransformOptions type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/kg-card-factory/src/CardFactory.ts`:
- Around line 61-63: Add a unit test that exercises the new early-return branch
where cardOutput.nodeType === 3 and cardOutput.nodeValue === '' so regressions
are caught: construct a minimal text-node-like object with nodeType 3 and
nodeValue '' and pass it through the CardFactory method that produces/returns
cardOutput (the code in CardFactory.ts that contains the early return), assert
that the exact object is returned and that wrapper/transform logic is not
invoked; mock or spy any wrapper functions to ensure they are not called.
---
Nitpick comments:
In `@packages/kg-card-factory/src/CardFactory.ts`:
- Around line 79-103: The three transformer methods absoluteToRelative,
relativeToAbsolute, and toTransformReady repeat the same defaultOptions +
Object.assign merging logic; extract that into a small helper (e.g.,
buildTransformOptions or mergeTransformOptions) that accepts the incoming
_options and returns Object.assign({}, {assetsOnly: false, siteUrl:
factoryOptions.siteUrl}, _options), then replace the repeated blocks in
absoluteToRelative, relativeToAbsolute, and toTransformReady to call the helper
and pass the resulting options into card.absoluteToRelative,
card.relativeToAbsolute, and card.toTransformReady respectively; ensure the
helper is defined alongside the CardFactory code so it can reference
factoryOptions.siteUrl and the CardTransformOptions type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 317c0e38-9e8b-4b29-89a9-8ec3fa019704
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
packages/kg-card-factory/.gitignorepackages/kg-card-factory/eslint.config.mjspackages/kg-card-factory/index.jspackages/kg-card-factory/package.jsonpackages/kg-card-factory/src/CardFactory.tspackages/kg-card-factory/src/index.tspackages/kg-card-factory/test/CardFactory.test.jspackages/kg-card-factory/test/CardFactory.test.tspackages/kg-card-factory/test/utils/assertions.tspackages/kg-card-factory/test/utils/index.tspackages/kg-card-factory/test/utils/overrides.jspackages/kg-card-factory/test/utils/overrides.tspackages/kg-card-factory/tsconfig.json
💤 Files with no reviewable changes (3)
- packages/kg-card-factory/test/CardFactory.test.js
- packages/kg-card-factory/test/utils/overrides.js
- packages/kg-card-factory/index.js
✅ Files skipped from review due to trivial changes (1)
- packages/kg-card-factory/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/kg-card-factory/test/CardFactory.test.ts
- packages/kg-card-factory/src/index.ts
- packages/kg-card-factory/.gitignore
- packages/kg-card-factory/test/utils/overrides.ts
kevinansfield
force-pushed
the
ts/kg-card-factory
branch
from
3bf164d to
23bdf80
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/kg-card-factory/package.json (2)
10-17: Verify theexportsmap condition ordering is intentional.The
"node"condition is placed before"import", which means Node.js will always resolve to the CommonJS build (build/cjs/index.js), even when using ESM imports. This is a valid choice for maximum compatibility, but if you want Node ESM users to get the ESM build, consider reordering:Optional: Prioritize ESM for Node
"exports": { ".": { "types": "./build/esm/index.d.ts", - "node": "./build/cjs/index.js", "import": "./build/esm/index.js", - "require": "./build/cjs/index.js" + "require": "./build/cjs/index.js", + "default": "./build/cjs/index.js" } },The current setup ensures CJS is used in Node for compatibility. If this is intentional for the Ghost ecosystem, the current configuration is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-card-factory/package.json` around lines 10 - 17, The exports map currently places the "node" condition before "import", causing Node to resolve to the CommonJS build (build/cjs/index.js) even for ESM imports; if you want Node ESM consumers to receive the ESM build instead, reorder the exports object so the "import" condition appears before "node" (i.e., ensure the keys under "." list "types", then "import", then "node", then "require"), updating the exports mapping in package.json accordingly.
20-22: Consider extracting the repeated build command.The
build,prepare, andpretestscripts are identical. While this works, you could simplify maintenance:Optional refactor
- "build": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", - "prepare": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", - "pretest": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", + "build": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", + "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-card-factory/package.json` around lines 20 - 22, The three identical npm scripts (build, prepare, pretest) are duplicated; define a single canonical script (e.g., "build") that runs "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json" and have the other scripts invoke it (e.g., set "prepare": "npm run build" and "pretest": "npm run build") so you maintain one source of truth; update the package.json scripts section accordingly, referencing the existing "build", "prepare", and "pretest" keys to wire them to the single command.packages/kg-card-factory/src/CardFactory.ts (1)
79-104: Consider extracting duplicatedefaultOptionsconstruction.The same
defaultOptionsobject pattern is repeated three times acrossabsoluteToRelative,relativeToAbsolute, andtoTransformReady. This is a minor DRY violation.♻️ Optional refactor to reduce duplication
+ private getDefaultTransformOptions(): CardTransformOptions { + return {assetsOnly: false, siteUrl: this.factoryOptions.siteUrl}; + } + createCard(card: CardDefinition) { const {factoryOptions} = this; + const getDefaultTransformOptions = () => ({assetsOnly: false, siteUrl: factoryOptions.siteUrl}); const {name, type, config = {}} = card; // ... absoluteToRelative(payload: CardPayload, _options?: CardTransformOptions) { if (card.absoluteToRelative) { - const defaultOptions = {assetsOnly: false, siteUrl: factoryOptions.siteUrl}; + const defaultOptions = getDefaultTransformOptions(); const options = Object.assign({}, defaultOptions, _options);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-card-factory/src/CardFactory.ts` around lines 79 - 104, The three methods absoluteToRelative, relativeToAbsolute, and toTransformReady duplicate construction of the same defaultOptions ({assetsOnly: false, siteUrl: factoryOptions.siteUrl}) and Object.assign merge with _options; extract this into a single helper or shared constant (e.g., createDefaultTransformOptions or defaultTransformOptions) within the CardFactory scope and use it in each method to build options before calling card.absoluteToRelative, card.relativeToAbsolute, or card.toTransformReady, preserving the Object.assign merge behavior with _options and referencing factoryOptions.siteUrl.
🤖 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-card-factory/test/CardFactory.test.ts`:
- Line 24: The test constructs opts with payload set to undefined which violates
the CardRenderArgs.payload type (CardPayload); update the test to pass a
CardPayload-compatible object (e.g., an empty object {}) for opts.payload so the
variable opts (used in the CardFactory tests) matches the CardRenderArgs type;
locate the variable named opts in CardFactory.test.ts and replace payload:
undefined with payload: {} (or a minimal CardPayload shape) to satisfy the
CardPayload type and avoid type errors.
- Line 42: The test constructs opts with payload: undefined which causes a type
mismatch; update the test to provide a correctly typed payload (e.g., payload:
null or a valid payload object) or use a type assertion to match the expected
type instead of undefined. Locate the opts declaration in CardFactory.test.ts
(the const opts = {env: {dom: new SimpleDomDocument()}, payload: undefined,
options: {}};) and replace payload: undefined with a properly typed value (or
cast: payload: undefined as unknown as ExpectedPayloadType) so the variable
matches the function/class signatures that consume opts.
---
Nitpick comments:
In `@packages/kg-card-factory/package.json`:
- Around line 10-17: The exports map currently places the "node" condition
before "import", causing Node to resolve to the CommonJS build
(build/cjs/index.js) even for ESM imports; if you want Node ESM consumers to
receive the ESM build instead, reorder the exports object so the "import"
condition appears before "node" (i.e., ensure the keys under "." list "types",
then "import", then "node", then "require"), updating the exports mapping in
package.json accordingly.
- Around line 20-22: The three identical npm scripts (build, prepare, pretest)
are duplicated; define a single canonical script (e.g., "build") that runs "tsc
&& tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' >
build/esm/package.json" and have the other scripts invoke it (e.g., set
"prepare": "npm run build" and "pretest": "npm run build") so you maintain one
source of truth; update the package.json scripts section accordingly,
referencing the existing "build", "prepare", and "pretest" keys to wire them to
the single command.
In `@packages/kg-card-factory/src/CardFactory.ts`:
- Around line 79-104: The three methods absoluteToRelative, relativeToAbsolute,
and toTransformReady duplicate construction of the same defaultOptions
({assetsOnly: false, siteUrl: factoryOptions.siteUrl}) and Object.assign merge
with _options; extract this into a single helper or shared constant (e.g.,
createDefaultTransformOptions or defaultTransformOptions) within the CardFactory
scope and use it in each method to build options before calling
card.absoluteToRelative, card.relativeToAbsolute, or card.toTransformReady,
preserving the Object.assign merge behavior with _options and referencing
factoryOptions.siteUrl.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac5b3826-e88f-4892-a00d-f03dee65d6a2
📒 Files selected for processing (14)
packages/kg-card-factory/.gitignorepackages/kg-card-factory/eslint.config.mjspackages/kg-card-factory/index.jspackages/kg-card-factory/package.jsonpackages/kg-card-factory/src/CardFactory.tspackages/kg-card-factory/src/index.tspackages/kg-card-factory/test/CardFactory.test.jspackages/kg-card-factory/test/CardFactory.test.tspackages/kg-card-factory/test/utils/assertions.tspackages/kg-card-factory/test/utils/index.tspackages/kg-card-factory/test/utils/overrides.jspackages/kg-card-factory/test/utils/overrides.tspackages/kg-card-factory/tsconfig.cjs.jsonpackages/kg-card-factory/tsconfig.json
💤 Files with no reviewable changes (3)
- packages/kg-card-factory/test/CardFactory.test.js
- packages/kg-card-factory/index.js
- packages/kg-card-factory/test/utils/overrides.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/kg-card-factory/.gitignore
- packages/kg-card-factory/src/index.ts
- packages/kg-card-factory/test/utils/overrides.ts
- packages/kg-card-factory/tsconfig.json
kevinansfield
force-pushed
the
ts/kg-card-factory
branch
3 times, most recently
from
199cc85 to
89707bf
Compare
kevinansfield
force-pushed
the
ts/kg-card-factory
branch
2 times, most recently
from
f8ac7f4 to
ee4f7d0
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-card-factory
branch
from
ee4f7d0 to
50e3ccf
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-card-factoryyarn lintpasses in kg-card-factory