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 converts packages/kg-converters to a TypeScript-first layout: adds tsconfig files, moves source to src/*.ts, and replaces Rollup/JS build assets with a single build output and an exports map in package.json. It adds strongly-typed implementations for lexical↔mobiledoc converters (src/lexical-to-mobiledoc.ts, src/mobiledoc-to-lexical.ts), re-exports them from src/index.ts, removes JS lib barrels and rollup.config.mjs, replaces ESLint flat config with a typescript-eslint wrapper, updates .gitignore to ignore build/, and migrates tests to TypeScript/ESM. 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 |
kevinansfield
force-pushed
the
ts/kg-converters
branch
from
a10fe48 to
1cf612a
Compare
kevinansfield
force-pushed
the
ts/kg-converters
branch
from
d220a52 to
5031400
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: 6
🧹 Nitpick comments (2)
packages/kg-converters/tsconfig.json (2)
12-12: PintsBuildInfoFilewhen using incremental buildsOn Line 12,
incremental: trueis good for local speed, but withouttsBuildInfoFilethe cache file location is less controlled. Consider placing it underbuild/to avoid workspace clutter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/tsconfig.json` at line 12, Add a pinned tsBuildInfoFile to the tsconfig so the incremental compiler output is stored in a controlled location (e.g., "build/tsconfig.tsbuildinfo"); update the tsconfig.json near the incremental: true setting by adding the tsBuildInfoFile property, referencing the same project name/intent, so incremental builds write their .tsbuildinfo into build/ instead of scattering files at workspace root.
5-5: All relative imports already use explicit.jsextensions; however, consider switching ESMmoduleResolutiontoNodeNextfor compile-time enforcementThe codebase correctly uses explicit
.jsextensions on all relative imports (3 total), which ensures Node ESM runtime compatibility despitebundlerresolution. Since this is a published library with dual ESM/CJS output, switching the ESMtsconfig.jsonfrommoduleResolution: "bundler"toNodeNextwould enforce this best practice at compile-time rather than relying on developer discipline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/tsconfig.json` at line 5, Update the ESM TypeScript config to use NodeNext resolution: change the tsconfig setting named "moduleResolution" from "bundler" to "NodeNext" so the compiler enforces explicit .js extensions for ESM imports; locate the JSON property "moduleResolution" in the tsconfig used for ESM output (the entry currently set to "bundler") and replace its value with "NodeNext".
🤖 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-converters/eslint.config.mjs`:
- Around line 18-19: The current config spreads ghostPlugin.configs.ts.rules
(and similarly ts-test) which are legacy .eslintrc presets and incompatible with
ESLint v9 flat-config; replace that spread usage by converting the legacy
presets via `@eslint/eslintrc`'s FlatCompat (create a FlatCompat instance, call
compat.extends('plugin:ghost/ts') and compat.extends('plugin:ghost/ts-test')
where needed) and then supply the returned arrays into the tseslint.config call
alongside your files/ rules blocks (leave your
'@typescript-eslint/no-explicit-any' rule in the appropriate files-scoped rules
object); ensure you import and instantiate FlatCompat with a correct
baseDirectory (use fileURLToPath(dirname) pattern) so the legacy plugin presets
are bridged into the flat config.
In `@packages/kg-converters/package.json`:
- Around line 10-16: The "node" conditional in the package.json exports
currently precedes "import"/"require" and thus shadows the ESM entry; update the
"." export object so the "import" (./build/esm/index.js) and "require"
(./build/cjs/index.js) keys come before "node" to ensure ESM imports resolve to
the ESM build and Node-specific fallback ("node": ./build/cjs/index.js) is only
used when appropriate; adjust the ordering of the keys "import", "require", and
"node" in the exports block accordingly.
In `@packages/kg-converters/src/mobiledoc-to-lexical.ts`:
- Around line 128-130: The callout handler currently casts
payload.backgroundColor to string and calls .match(), which will throw for
non-string truthy values; update the callout function to first check that
payload.backgroundColor is a string (e.g., typeof payload.backgroundColor ===
'string') before invoking .match(), and if it is not a string or the regex test
fails, set payload.backgroundColor = 'white'; ensure you only perform the regex
match on a validated string to avoid runtime errors during conversion.
- Around line 246-258: The link attributes (href and rel) are reused across
iterations causing rel to bleed into subsequent links; inside the
openMarkupsIndexes.forEach handler (where openLinkMarkup, href, rel and markup
are referenced) reset href and rel to undefined (or empty string per project
conventions) at the start of each iteration or immediately when you detect a new
'a' markup before reading attrs so that a link without a rel does not inherit
the previous rel value.
In `@packages/kg-converters/test/exports.test.ts`:
- Around line 1-18: The test imports converters from the source barrel
(lexicalToMobiledoc, mobiledocToLexical) which doesn't validate the
published/build entrypoints; change the import in
packages/kg-converters/test/exports.test.ts to require the package's built
entrypoint (the package root/main/exports) instead of ../src/index.js so the
test exercises package.json#exports and the generated build files; keep the same
assertions and symbol names (lexicalToMobiledoc, mobiledocToLexical) and run the
test to ensure the built entrypoint exports both functions.
In `@packages/kg-converters/tsconfig.cjs.json`:
- Line 5: The tsconfig CJS config currently sets "moduleResolution": "Node10",
which ignores package.json "exports" conditional fields; update the
moduleResolution option in packages/kg-converters/tsconfig.cjs.json from
"Node10" to "Node16" (or "nodenext") so TypeScript will honor package.json
exports and conditional resolution for CJS builds—change the "moduleResolution"
property value and run a quick build/test to verify no downstream resolution
breaks.
---
Nitpick comments:
In `@packages/kg-converters/tsconfig.json`:
- Line 12: Add a pinned tsBuildInfoFile to the tsconfig so the incremental
compiler output is stored in a controlled location (e.g.,
"build/tsconfig.tsbuildinfo"); update the tsconfig.json near the incremental:
true setting by adding the tsBuildInfoFile property, referencing the same
project name/intent, so incremental builds write their .tsbuildinfo into build/
instead of scattering files at workspace root.
- Line 5: Update the ESM TypeScript config to use NodeNext resolution: change
the tsconfig setting named "moduleResolution" from "bundler" to "NodeNext" so
the compiler enforces explicit .js extensions for ESM imports; locate the JSON
property "moduleResolution" in the tsconfig used for ESM output (the entry
currently set to "bundler") and replace its value with "NodeNext".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6043eff7-02ef-430f-a5ae-1929f7356b8f
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
packages/kg-converters/.gitignorepackages/kg-converters/eslint.config.mjspackages/kg-converters/index.jspackages/kg-converters/lib/kg-converters.jspackages/kg-converters/package.jsonpackages/kg-converters/rollup.config.mjspackages/kg-converters/src/index.tspackages/kg-converters/src/kg-converters.tspackages/kg-converters/src/lexical-to-mobiledoc.tspackages/kg-converters/src/mobiledoc-to-lexical.tspackages/kg-converters/test/exports.test.jspackages/kg-converters/test/exports.test.tspackages/kg-converters/test/lexical-to-mobiledoc.test.tspackages/kg-converters/test/mobiledoc-to-lexical.test.tspackages/kg-converters/tsconfig.cjs.jsonpackages/kg-converters/tsconfig.json
💤 Files with no reviewable changes (4)
- packages/kg-converters/test/exports.test.js
- packages/kg-converters/index.js
- packages/kg-converters/rollup.config.mjs
- packages/kg-converters/lib/kg-converters.js
Comment on lines
+18
to
+19
| ...ghostPlugin.configs.ts.rules, | ||
| '@typescript-eslint/no-explicit-any': 'error', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
plugin_dir="$(fd '^eslint-plugin-ghost$' packages -t d | head -n1)"
if [ -z "$plugin_dir" ]; then
echo "Could not locate eslint-plugin-ghost under packages/" >&2
exit 1
fi
rg -n -C8 "\\bts\\b\\s*:\\s*\\{|['\\\"]ts-test['\\\"]\\s*:\\s*\\{|languageOptions\\s*:|plugins\\s*:|settings\\s*:|ignores\\s*:" "$plugin_dir" -g '!**/build/**'Repository: TryGhost/Koenig
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Check if eslint-plugin-ghost is listed as an external dependency
echo "=== Checking package.json for eslint-plugin-ghost ==="
fd package.json packages/kg-converters -x cat {} | grep -A10 -B10 "eslint-plugin-ghost" || echo "Not found in direct search"
# Display the full eslint.config.mjs file to see the imports and context
echo -e "\n=== Full eslint.config.mjs content ==="
cat packages/kg-converters/eslint.config.mjs
# Check if there's a monorepo root package.json
echo -e "\n=== Checking root package.json for eslint-plugin-ghost ==="
cat package.json | grep -A5 -B5 "eslint-plugin-ghost" || echo "Not found in root"Repository: TryGhost/Koenig
Length of output: 1368
🌐 Web query:
eslint-plugin-ghost npm preset config ts rules languageOptions
💡 Result:
eslint-plugin-ghost ships legacy .eslintrc* “preset configs” (exposed as plugin:ghost/<preset>), not ESLint v9 flat-config languageOptions objects. Usage is via extends: ['plugin:ghost/<preset>'] (presets include node, browser, es, ember, ts, test, filenames, plus *-no-style variants, and ts-test). [1]
What the ts preset does (rules / parser options)
The plugin:ghost/ts preset:
- sets
env: { node: true, es6: true } - sets
parserOptions: { ecmaVersion: 2022 } - enables
@typescript-eslintand extendsplugin:@typescript-eslint/recommended`` - merges Ghost’s shared rules and disables
@typescript-eslint/no-inferrable-types[2]
plugin:ghost/ts-test extends ./ts and adds ./test-rules. [3]
ESLint v9 flat config (languageOptions) equivalent (if you want to avoid .eslintrc)
If you’re using eslint.config.js, you typically bring these legacy presets in via FlatCompat, then add/override languageOptions as needed (especially the TS parser):
// eslint.config.js
import { FlatCompat } from '@eslint/eslintrc'
import tsParser from '@typescript-eslint/parser'
const compat = new FlatCompat({ baseDirectory: import.meta.dirname })
export default [
...compat.extends('plugin:ghost/ts'),
{
files: ['**/*.{ts,tsx}'],
languageOptions: {
parser: tsParser,
parserOptions: { ecmaVersion: 2022 },
},
},
](Typescript-eslint’s flat-config docs show languageOptions.parser / languageOptions.parserOptions as the place to configure TS parsing in flat config.) [4]
Sources: [1] [2] [3] [4]
Replace spread of Ghost plugin rules with proper ESLint v9 flat-config integration.
eslint-plugin-ghost v3.5.0 exports legacy .eslintrc* presets (e.g., plugin:ghost/ts, plugin:ghost/ts-test), not ESLint v9 flat-config objects. Accessing ghostPlugin.configs.ts.rules and ghostPlugin.configs['ts-test'].rules is not compatible with the plugin's actual structure and will not work reliably.
Use @eslint/eslintrc's FlatCompat to bridge the legacy presets into flat config, or migrate to .eslintrc.json format. If using FlatCompat:
♻️ Example using FlatCompat
import { FlatCompat } from '@eslint/eslintrc';
import tseslint from 'typescript-eslint';
import path from 'path';
import { fileURLToPath } from 'url';
const dirname = path.dirname(fileURLToPath(import.meta.url));
const compat = new FlatCompat({ baseDirectory: dirname });
export default tseslint.config([
{ ignores: ['build/**'] },
...compat.extends('plugin:ghost/ts'),
{
files: ['**/*.ts'],
rules: {
'@typescript-eslint/no-explicit-any': 'error',
},
},
{
files: ['test/**/*.ts'],
...compat.extends('plugin:ghost/ts-test'),
rules: {
'ghost/mocha/no-global-tests': 'off',
'ghost/mocha/handle-done-callback': 'off',
'ghost/mocha/no-mocha-arrows': 'off',
'ghost/mocha/max-top-level-suites': 'off',
},
},
]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-converters/eslint.config.mjs` around lines 18 - 19, The current
config spreads ghostPlugin.configs.ts.rules (and similarly ts-test) which are
legacy .eslintrc presets and incompatible with ESLint v9 flat-config; replace
that spread usage by converting the legacy presets via `@eslint/eslintrc`'s
FlatCompat (create a FlatCompat instance, call compat.extends('plugin:ghost/ts')
and compat.extends('plugin:ghost/ts-test') where needed) and then supply the
returned arrays into the tseslint.config call alongside your files/ rules blocks
(leave your '@typescript-eslint/no-explicit-any' rule in the appropriate
files-scoped rules object); ensure you import and instantiate FlatCompat with a
correct baseDirectory (use fileURLToPath(dirname) pattern) so the legacy plugin
presets are bridged into the flat config.
Comment on lines
+246
to
258
| openMarkupsIndexes.forEach((markupIndex: number) => { | ||
| const markup = markups[markupIndex]; | ||
| // Extract the href from the markup if it's a link | ||
| if (markup[0] === 'a') { | ||
| openLinkMarkup = true; | ||
| if (markup[1] && markup[1][0] === 'href') { | ||
| href = markup[1][1]; | ||
| const attrs = markup[1] as unknown[] | undefined; | ||
| if (attrs && attrs[0] === 'href') { | ||
| href = attrs[1] as string; | ||
| } | ||
|
|
||
| if (markup[1] && markup[1][2] === 'rel') { | ||
| rel = markup[1][3]; | ||
| if (attrs && attrs[2] === 'rel') { | ||
| rel = attrs[3] as string; | ||
| } |
There was a problem hiding this comment.
Reset link attributes before reusing them.
rel is section-scoped state here. If one link has rel and the next link does not, the second link inherits the first link's rel because nothing clears it before the next <a> is parsed.
Proposed fix
if (markup[0] === 'a') {
openLinkMarkup = true;
+ href = undefined;
+ rel = undefined;
const attrs = markup[1] as unknown[] | undefined;
if (attrs && attrs[0] === 'href') {
href = attrs[1] as string;
@@
if (markup && markup[0] === 'a') {
embedChildNode(lexicalNode, linkNode);
openLinkMarkup = false;
href = undefined;
+ rel = undefined;
linkNode = undefined;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-converters/src/mobiledoc-to-lexical.ts` around lines 246 - 258,
The link attributes (href and rel) are reused across iterations causing rel to
bleed into subsequent links; inside the openMarkupsIndexes.forEach handler
(where openLinkMarkup, href, rel and markup are referenced) reset href and rel
to undefined (or empty string per project conventions) at the start of each
iteration or immediately when you detect a new 'a' markup before reading attrs
so that a link without a rel does not inherit the previous rel value.
Comment on lines
+1
to
+18
| import assert from 'assert/strict'; | ||
| import {lexicalToMobiledoc, mobiledocToLexical} from '../src/index.js'; | ||
|
|
||
| describe('Exports', function () { | ||
| it('includes both converter functions', function () { | ||
| assert.ok(lexicalToMobiledoc); | ||
| assert.equal(typeof lexicalToMobiledoc, 'function'); | ||
| assert.ok(mobiledocToLexical); | ||
| assert.equal(typeof mobiledocToLexical, 'function'); | ||
| }); | ||
|
|
||
| it('lexicalToMobiledoc runs without error', function () { | ||
| assert.ok(lexicalToMobiledoc('{}')); | ||
| }); | ||
|
|
||
| it('mobiledocToLexical runs without error', function () { | ||
| assert.ok(mobiledocToLexical('{}')); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Point this suite at the built entrypoints, not src/.
Importing ../src/index.js only proves the source barrel works under tsx; it does not exercise package.json#exports, main, or the generated build/* files. A broken publish surface would still pass this test.
Proposed fix
import assert from 'assert/strict';
-import {lexicalToMobiledoc, mobiledocToLexical} from '../src/index.js';
+import {createRequire} from 'node:module';
+import {lexicalToMobiledoc, mobiledocToLexical} from '../build/esm/index.js';
+
+const require = createRequire(import.meta.url);
+const cjsExports = require('../build/cjs/index.js');
@@
it('includes both converter functions', function () {
assert.ok(lexicalToMobiledoc);
assert.equal(typeof lexicalToMobiledoc, 'function');
assert.ok(mobiledocToLexical);
assert.equal(typeof mobiledocToLexical, 'function');
+ assert.equal(typeof cjsExports.lexicalToMobiledoc, 'function');
+ assert.equal(typeof cjsExports.mobiledocToLexical, 'function');
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-converters/test/exports.test.ts` around lines 1 - 18, The test
imports converters from the source barrel (lexicalToMobiledoc,
mobiledocToLexical) which doesn't validate the published/build entrypoints;
change the import in packages/kg-converters/test/exports.test.ts to require the
package's built entrypoint (the package root/main/exports) instead of
../src/index.js so the test exercises package.json#exports and the generated
build files; keep the same assertions and symbol names (lexicalToMobiledoc,
mobiledocToLexical) and run the test to ensure the built entrypoint exports both
functions.
e550854 to
9194eb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
packages/kg-converters/package.json (1)
13-15:⚠️ Potential issue | 🟠 MajorReorder conditional exports to avoid ESM/CJS shadowing.
At Line 13,
"node"is checked before"import"/"require", which can route ESM imports to CJS in Node due to condition-order precedence.In Node.js package.json conditional exports, does object key order control precedence, and if "node" appears before "import", which target is chosen for an ESM import?Proposed fix
"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/esm/index.js" } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/package.json` around lines 13 - 15, The conditional export object currently has "node" before "import", which can make ESM imports resolve to the CJS target; reorder the keys so "import" appears before "node" (e.g., "import", "node", "require") in the package.json exports to ensure ESM consumers pick the ESM bundle, and keep "require" pointing at the CJS build; update the export object keys "import", "node", and "require" accordingly.packages/kg-converters/test/exports.test.ts (1)
2-18: 🛠️ Refactor suggestion | 🟠 MajorExercise the built package entrypoints, not
src/.Importing
../src/index.jsonly proves the source barrel loads in tests. It will not catch a brokenpackage.json#exports,main, or generated CJS/ESM output, which is the surface this PR changes. Point this suite at the built package surface, or the package root after build, instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/test/exports.test.ts` around lines 2 - 18, The tests import the source barrel (../src/index.js) instead of the built package entrypoint so they don't validate package.json exports or generated outputs; update the import in the test that references lexicalToMobiledoc and mobiledocToLexical to import from the package's built entry (the package root / published entrypoint) instead of '../src/index.js' so the suite exercises the actual distributed CJS/ESM output and package.json#exports.packages/kg-converters/src/mobiledoc-to-lexical.ts (2)
246-258:⚠️ Potential issue | 🟠 MajorReset
relwhen opening and closing links.
relis section-scoped state here. If one link hasreland the next one doesn't, the second link inherits the first link's value.Suggested fix
openMarkupsIndexes.forEach((markupIndex: number) => { const markup = markups[markupIndex]; // Extract the href from the markup if it's a link if (markup[0] === 'a') { openLinkMarkup = true; + href = undefined; + rel = undefined; const attrs = markup[1] as unknown[] | undefined; if (attrs && attrs[0] === 'href') { href = attrs[1] as string; @@ if (markup && markup[0] === 'a') { embedChildNode(lexicalNode, linkNode); openLinkMarkup = false; href = undefined; + rel = undefined; linkNode = undefined; }Also applies to: 291-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/mobiledoc-to-lexical.ts` around lines 246 - 258, openMarkupsIndexes.forEach loop is keeping section-scoped rel state so a link without rel inherits the previous value; update the logic in the open-markup handling (inside openMarkupsIndexes.forEach) to explicitly reset rel when starting a new link that has no rel attribute (set rel to undefined or empty string when attrs is missing or attrs[2] !== 'rel'), and likewise ensure rel is cleared when closing the link (where openLinkMarkup is set false) so subsequent links do not inherit the previous rel; reference variables: openMarkupsIndexes.forEach, markup, openLinkMarkup, href, rel to locate where to add the resets.
127-130:⚠️ Potential issue | 🟠 MajorValidate
backgroundColorbefore calling.match().A truthy non-string
payload.backgroundColorstill throws here and aborts the whole conversion.Suggested fix
callout: (payload: Record<string, unknown>) => { - if (payload.backgroundColor && !(payload.backgroundColor as string).match(/^[a-zA-Z\d-]+$/)) { + if ( + payload.backgroundColor !== undefined && + (typeof payload.backgroundColor !== 'string' || !/^[a-zA-Z\d-]+$/.test(payload.backgroundColor)) + ) { payload.backgroundColor = 'white'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/mobiledoc-to-lexical.ts` around lines 127 - 130, The callout card handler in CARD_FIXES_MAP calls payload.backgroundColor.match() without ensuring backgroundColor is a string, causing runtime errors for non-string truthy values; update the callout fixer (the callout function inside CARD_FIXES_MAP) to first check that typeof payload.backgroundColor === 'string' (or safely coerce to string) before using .match(), and if the check fails or the regex doesn't match, set payload.backgroundColor = 'white' as the fallback.
🧹 Nitpick comments (2)
packages/kg-converters/eslint.config.mjs (1)
22-31: Test config relies on plugin registration from the preceding config block.The test config at lines 22-31 doesn't register the
ghostplugin but referencesghost/mocha/*rules. This works because ESLint flat config merges configs for files matching multiple patterns, sotest/**/*.tsfiles inherit theghostplugin from the**/*.tsconfig block. This is valid but worth noting for maintainability—if the main config changes, the test rules could break.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/eslint.config.mjs` around lines 22 - 31, The test files block references rules under the ghost plugin (e.g., 'ghost/mocha/*') but doesn't register ghostPlugin, relying on inheritance from the earlier "**/*.ts" block; make the test config explicitly register the plugin so it doesn't break if the other block changes — update the files: ['test/**/*.ts'] config object to include the ghost plugin registration (use the existing ghostPlugin identifier) alongside the rules in that block so 'ghost/mocha/*' rules resolve locally.packages/kg-converters/package.json (1)
20-22: Deduplicate repeated build pipeline command.Lines 20-22 repeat the same command string three times; this is easy to drift and harder to maintain.
Proposed refactor
"scripts": { "dev": "tsc --watch --preserveWatchOutput", - "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:artifacts": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", + "build": "yarn build:artifacts", + "prepare": "yarn build:artifacts", + "pretest": "yarn build:artifacts",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/package.json` around lines 20 - 22, The package.json repeats the same command in the "build", "prepare", and "pretest" scripts; add a single canonical script (e.g., "build:all") containing the current command string and update "build", "prepare", and "pretest" to invoke that script (e.g., "npm run build:all") so the build command is defined once; look for the "build", "prepare", and "pretest" script entries in package.json to change them to reference the new "build:all" 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-converters/src/lexical-to-mobiledoc.ts`:
- Around line 301-303: The link markup currently only serializes href causing
rel to be lost on round-trip; when handling child.type === 'link' in
lexical-to-mobiledoc.ts (the code that builds linkMarkup and pushes to
mobiledoc.markups), include the rel attribute if present on child (child.rel) in
the markup attributes array alongside href before pushing linkMarkup to
mobiledoc.markups so the rel metadata is preserved for mobiledoc-to-lexical.ts
to read back.
- Around line 112-120: In lexicalToMobiledoc, JSON.parse(serializedLexical) can
return the literal null (e.g. when serializedLexical === "null"), so avoid
accessing lexical.root without checking lexical itself; after parsing, validate
that lexical is non-null and that lexical.root exists (e.g., if (!lexical ||
!lexical.root) return JSON.stringify(BLANK_DOC)); update the parsing flow around
the const lexical = JSON.parse(...) and the subsequent if that checks .root to
handle the parsed null case.
In `@packages/kg-converters/src/mobiledoc-to-lexical.ts`:
- Around line 137-145: The function mobiledocToLexical currently parses
serializedMobiledoc and immediately accesses mobiledoc.sections which will throw
if JSON.parse returns the literal null; update mobiledocToLexical to validate
the parsed mobiledoc (the variable mobiledoc) immediately after JSON.parse — if
mobiledoc is null or not an object or mobiledoc.sections is falsy, return
JSON.stringify(BLANK_DOC); keep the rest of the conversion logic unchanged.
---
Duplicate comments:
In `@packages/kg-converters/package.json`:
- Around line 13-15: The conditional export object currently has "node" before
"import", which can make ESM imports resolve to the CJS target; reorder the keys
so "import" appears before "node" (e.g., "import", "node", "require") in the
package.json exports to ensure ESM consumers pick the ESM bundle, and keep
"require" pointing at the CJS build; update the export object keys "import",
"node", and "require" accordingly.
In `@packages/kg-converters/src/mobiledoc-to-lexical.ts`:
- Around line 246-258: openMarkupsIndexes.forEach loop is keeping section-scoped
rel state so a link without rel inherits the previous value; update the logic in
the open-markup handling (inside openMarkupsIndexes.forEach) to explicitly reset
rel when starting a new link that has no rel attribute (set rel to undefined or
empty string when attrs is missing or attrs[2] !== 'rel'), and likewise ensure
rel is cleared when closing the link (where openLinkMarkup is set false) so
subsequent links do not inherit the previous rel; reference variables:
openMarkupsIndexes.forEach, markup, openLinkMarkup, href, rel to locate where to
add the resets.
- Around line 127-130: The callout card handler in CARD_FIXES_MAP calls
payload.backgroundColor.match() without ensuring backgroundColor is a string,
causing runtime errors for non-string truthy values; update the callout fixer
(the callout function inside CARD_FIXES_MAP) to first check that typeof
payload.backgroundColor === 'string' (or safely coerce to string) before using
.match(), and if the check fails or the regex doesn't match, set
payload.backgroundColor = 'white' as the fallback.
In `@packages/kg-converters/test/exports.test.ts`:
- Around line 2-18: The tests import the source barrel (../src/index.js) instead
of the built package entrypoint so they don't validate package.json exports or
generated outputs; update the import in the test that references
lexicalToMobiledoc and mobiledocToLexical to import from the package's built
entry (the package root / published entrypoint) instead of '../src/index.js' so
the suite exercises the actual distributed CJS/ESM output and
package.json#exports.
---
Nitpick comments:
In `@packages/kg-converters/eslint.config.mjs`:
- Around line 22-31: The test files block references rules under the ghost
plugin (e.g., 'ghost/mocha/*') but doesn't register ghostPlugin, relying on
inheritance from the earlier "**/*.ts" block; make the test config explicitly
register the plugin so it doesn't break if the other block changes — update the
files: ['test/**/*.ts'] config object to include the ghost plugin registration
(use the existing ghostPlugin identifier) alongside the rules in that block so
'ghost/mocha/*' rules resolve locally.
In `@packages/kg-converters/package.json`:
- Around line 20-22: The package.json repeats the same command in the "build",
"prepare", and "pretest" scripts; add a single canonical script (e.g.,
"build:all") containing the current command string and update "build",
"prepare", and "pretest" to invoke that script (e.g., "npm run build:all") so
the build command is defined once; look for the "build", "prepare", and
"pretest" script entries in package.json to change them to reference the new
"build:all" script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8199cdc2-ba7e-4528-bfbf-a229381692ce
📒 Files selected for processing (16)
packages/kg-converters/.gitignorepackages/kg-converters/eslint.config.mjspackages/kg-converters/index.jspackages/kg-converters/lib/kg-converters.jspackages/kg-converters/package.jsonpackages/kg-converters/rollup.config.mjspackages/kg-converters/src/index.tspackages/kg-converters/src/kg-converters.tspackages/kg-converters/src/lexical-to-mobiledoc.tspackages/kg-converters/src/mobiledoc-to-lexical.tspackages/kg-converters/test/exports.test.jspackages/kg-converters/test/exports.test.tspackages/kg-converters/test/lexical-to-mobiledoc.test.tspackages/kg-converters/test/mobiledoc-to-lexical.test.tspackages/kg-converters/tsconfig.cjs.jsonpackages/kg-converters/tsconfig.json
💤 Files with no reviewable changes (4)
- packages/kg-converters/test/exports.test.js
- packages/kg-converters/lib/kg-converters.js
- packages/kg-converters/rollup.config.mjs
- packages/kg-converters/index.js
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/kg-converters/src/kg-converters.ts
- packages/kg-converters/tsconfig.json
- packages/kg-converters/tsconfig.cjs.json
- packages/kg-converters/src/index.ts
- packages/kg-converters/test/lexical-to-mobiledoc.test.ts
- packages/kg-converters/test/mobiledoc-to-lexical.test.ts
- packages/kg-converters/.gitignore
kevinansfield
force-pushed
the
ts/kg-converters
branch
from
9194eb6 to
6cc6497
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/kg-converters/src/lexical-to-mobiledoc.ts (2)
301-303:⚠️ Potential issue | 🟠 MajorPreserve link
relwhen serializing to Mobiledoc markup.Line 302 only writes
href, so round-tripping dropsrelmetadata that the reverse converter already reads.Proposed fix
- const linkMarkup: MobiledocMarkup = ['a', ['href', child.url]]; + const attrs: unknown[] = ['href', child.url]; + if (typeof child.rel === 'string' && child.rel) { + attrs.push('rel', child.rel); + } + const linkMarkup: MobiledocMarkup = ['a', attrs];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/lexical-to-mobiledoc.ts` around lines 301 - 303, The link markup currently only writes href and drops rel metadata; update the serialization in the lexical-to-mobiledoc converter where child.type === 'link' (the linkMarkup and its push into mobiledoc.markups) to include rel when present (e.g., add ['rel', child.rel] alongside ['href', child.url]) so the linkMarkup array preserves rel for round-tripping back to the reverse converter.
117-120:⚠️ Potential issue | 🟠 MajorHandle parsed
nullbefore reading.root.At Line 119,
JSON.parse('null')leads to a null dereference instead of returning the blank Mobiledoc.Proposed fix
- const lexical = JSON.parse(serializedLexical) as {root?: {children?: LexicalNode[]}}; - - if (!lexical.root) { + const lexical = JSON.parse(serializedLexical) as {root?: {children?: LexicalNode[]}} | null; + + if (!lexical?.root) { return JSON.stringify(BLANK_DOC); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/lexical-to-mobiledoc.ts` around lines 117 - 120, The code currently assumes JSON.parse(serializedLexical) returns an object and directly checks lexical.root, which throws if serializedLexical is "null"; change the guard to handle a null parse result before reading .root: after parsing serializedLexical into the lexical variable, ensure you check that lexical is truthy (e.g., lexical !== null) and that lexical.root exists (e.g., if (!lexical || !lexical.root) return JSON.stringify(BLANK_DOC)); update any type assertions as needed to perform a proper runtime null check on lexical before accessing .root to avoid the null dereference.packages/kg-converters/src/mobiledoc-to-lexical.ts (3)
127-130:⚠️ Potential issue | 🟠 MajorGuard
backgroundColortype before calling.match().Line 129 still does a cast-only check; a truthy non-string value will throw and stop conversion.
Proposed fix
const CARD_FIXES_MAP: Record<string, (payload: Record<string, unknown>) => Record<string, unknown>> = { callout: (payload: Record<string, unknown>) => { - if (payload.backgroundColor && !(payload.backgroundColor as string).match(/^[a-zA-Z\d-]+$/)) { + if ( + payload.backgroundColor !== undefined && + (typeof payload.backgroundColor !== 'string' || !/^[a-zA-Z\d-]+$/.test(payload.backgroundColor)) + ) { payload.backgroundColor = 'white'; } return payload; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/mobiledoc-to-lexical.ts` around lines 127 - 130, In the callout entry of CARD_FIXES_MAP, guard backgroundColor's type before calling .match(): instead of casting to string and calling match unconditionally in the callout handler, first check typeof payload.backgroundColor === 'string' (and truthiness) before using .match(), and only set payload.backgroundColor = 'white' when the string fails the regex; update the callout handler inside CARD_FIXES_MAP accordingly to avoid throwing on non-string values.
142-145:⚠️ Potential issue | 🟠 MajorHandle parsed
nullbefore reading.sections.At Line 144,
JSON.parse('null')makesmobiledocnull, so property access throws before the fallback path is reached.Proposed fix
- const mobiledoc = JSON.parse(serializedMobiledoc) as MobiledocDocument; - - if (!mobiledoc.sections) { + const mobiledoc = JSON.parse(serializedMobiledoc) as MobiledocDocument | null; + + if (!Array.isArray(mobiledoc?.sections)) { return JSON.stringify(BLANK_DOC); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/mobiledoc-to-lexical.ts` around lines 142 - 145, The code parses serializedMobiledoc into the variable mobiledoc and immediately reads mobiledoc.sections, which will throw if JSON.parse returns null (e.g., JSON "null"); update the logic that follows the JSON.parse call (the mobiledoc variable usage) to first check whether mobiledoc is truthy (non-null) before accessing mobiledoc.sections and return JSON.stringify(BLANK_DOC) when mobiledoc is null or when mobiledoc.sections is missing; locate the JSON.parse(serializedMobiledoc) assignment and the subsequent conditional that references mobiledoc.sections to implement this null guard.
246-258:⚠️ Potential issue | 🟠 MajorReset link-scoped
href/relstate between link markups.Lines 246-258 reuse section-scoped state, and Line 295 close logic still doesn’t clear
rel. A link withoutrelcan inherit a previous one.Proposed fix
openMarkupsIndexes.forEach((markupIndex: number) => { const markup = markups[markupIndex]; // Extract the href from the markup if it's a link if (markup[0] === 'a') { openLinkMarkup = true; + href = undefined; + rel = undefined; const attrs = markup[1] as unknown[] | undefined; if (attrs && attrs[0] === 'href') { href = attrs[1] as string; } @@ if (markup && markup[0] === 'a') { embedChildNode(lexicalNode, linkNode); openLinkMarkup = false; href = undefined; + rel = undefined; linkNode = undefined; }Also applies to: 291-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/mobiledoc-to-lexical.ts` around lines 246 - 258, The code reuses section-scoped link state (href and rel) across iterations causing a link without rel to inherit a previous value; in the loop that iterates openMarkupsIndexes (and in the link-close logic around where openLinkMarkup is cleared) reset href and rel to undefined before processing a new markup and also explicitly set rel = undefined when closing a link; update the logic inside the block handling markups[markupIndex] (where markup[0] === 'a', openLinkMarkup, attrs, href, rel are used) and the corresponding closing branch that clears openLinkMarkup to ensure href and rel are cleared so state doesn’t leak between link markups.
🧹 Nitpick comments (1)
packages/kg-converters/package.json (1)
19-22: Consider DRYing up the duplicated build commands.The
build,prepare, andpretestscripts contain identical commands. You could haveprepareandpretestinvokebuildinstead:Suggested refactor
"dev": "tsc --watch --preserveWatchOutput", "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", + "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-converters/package.json` around lines 19 - 22, The package.json scripts duplicate the same build command for "build", "prepare", and "pretest"; keep "build" unchanged and make "prepare" and "pretest" call that script (e.g., replace their command strings with "npm run build") so you DRY up the commands while preserving the existing build behavior for 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-converters/package.json`:
- Line 40: The package.json devDependencies are missing TypeScript typings for
sinon; add `@types/sinon` to devDependencies so tests using sinon in TypeScript
compile. Update packages/kg-converters package.json by adding an entry for
"@types/sinon" under devDependencies (choose a compatible version for sinon 21.x
or use a caret range) and run install to update lockfiles; ensure imports in
tests continue to reference sinon as before.
---
Duplicate comments:
In `@packages/kg-converters/src/lexical-to-mobiledoc.ts`:
- Around line 301-303: The link markup currently only writes href and drops rel
metadata; update the serialization in the lexical-to-mobiledoc converter where
child.type === 'link' (the linkMarkup and its push into mobiledoc.markups) to
include rel when present (e.g., add ['rel', child.rel] alongside ['href',
child.url]) so the linkMarkup array preserves rel for round-tripping back to the
reverse converter.
- Around line 117-120: The code currently assumes JSON.parse(serializedLexical)
returns an object and directly checks lexical.root, which throws if
serializedLexical is "null"; change the guard to handle a null parse result
before reading .root: after parsing serializedLexical into the lexical variable,
ensure you check that lexical is truthy (e.g., lexical !== null) and that
lexical.root exists (e.g., if (!lexical || !lexical.root) return
JSON.stringify(BLANK_DOC)); update any type assertions as needed to perform a
proper runtime null check on lexical before accessing .root to avoid the null
dereference.
In `@packages/kg-converters/src/mobiledoc-to-lexical.ts`:
- Around line 127-130: In the callout entry of CARD_FIXES_MAP, guard
backgroundColor's type before calling .match(): instead of casting to string and
calling match unconditionally in the callout handler, first check typeof
payload.backgroundColor === 'string' (and truthiness) before using .match(), and
only set payload.backgroundColor = 'white' when the string fails the regex;
update the callout handler inside CARD_FIXES_MAP accordingly to avoid throwing
on non-string values.
- Around line 142-145: The code parses serializedMobiledoc into the variable
mobiledoc and immediately reads mobiledoc.sections, which will throw if
JSON.parse returns null (e.g., JSON "null"); update the logic that follows the
JSON.parse call (the mobiledoc variable usage) to first check whether mobiledoc
is truthy (non-null) before accessing mobiledoc.sections and return
JSON.stringify(BLANK_DOC) when mobiledoc is null or when mobiledoc.sections is
missing; locate the JSON.parse(serializedMobiledoc) assignment and the
subsequent conditional that references mobiledoc.sections to implement this null
guard.
- Around line 246-258: The code reuses section-scoped link state (href and rel)
across iterations causing a link without rel to inherit a previous value; in the
loop that iterates openMarkupsIndexes (and in the link-close logic around where
openLinkMarkup is cleared) reset href and rel to undefined before processing a
new markup and also explicitly set rel = undefined when closing a link; update
the logic inside the block handling markups[markupIndex] (where markup[0] ===
'a', openLinkMarkup, attrs, href, rel are used) and the corresponding closing
branch that clears openLinkMarkup to ensure href and rel are cleared so state
doesn’t leak between link markups.
---
Nitpick comments:
In `@packages/kg-converters/package.json`:
- Around line 19-22: The package.json scripts duplicate the same build command
for "build", "prepare", and "pretest"; keep "build" unchanged and make "prepare"
and "pretest" call that script (e.g., replace their command strings with "npm
run build") so you DRY up the commands while preserving the existing build
behavior for the "build" script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6a2e323-a287-41d4-8b66-f221a8cc04fa
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
packages/kg-converters/.gitignorepackages/kg-converters/eslint.config.mjspackages/kg-converters/index.jspackages/kg-converters/package.jsonpackages/kg-converters/rollup.config.mjspackages/kg-converters/src/index.tspackages/kg-converters/src/kg-converters.tspackages/kg-converters/src/lexical-to-mobiledoc.tspackages/kg-converters/src/mobiledoc-to-lexical.tspackages/kg-converters/test/exports.test.tspackages/kg-converters/test/lexical-to-mobiledoc.test.tspackages/kg-converters/test/mobiledoc-to-lexical.test.tspackages/kg-converters/tsconfig.cjs.jsonpackages/kg-converters/tsconfig.json
💤 Files with no reviewable changes (2)
- packages/kg-converters/index.js
- packages/kg-converters/rollup.config.mjs
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/kg-converters/src/index.ts
- packages/kg-converters/test/lexical-to-mobiledoc.test.ts
- packages/kg-converters/test/exports.test.ts
- packages/kg-converters/.gitignore
- packages/kg-converters/src/kg-converters.ts
- packages/kg-converters/tsconfig.json
- packages/kg-converters/tsconfig.cjs.json
- packages/kg-converters/eslint.config.mjs
packages/kg-converters/package.json
Outdated
| "mocha": "11.7.5", | ||
| "rollup": "4.59.0", | ||
| "sinon": "21.0.2" | ||
| "sinon": "21.0.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does sinon 21.x include bundled TypeScript types or is @types/sinon needed?
💡 Result:
Sinon 21.x does not bundle TypeScript declaration files, so in a TypeScript project you should still install @types/sinon.
Evidence: the sinon@21.0.0 package.json has no types/typings entry and its published files list doesn’t include any .d.ts declarations. [1] The type declarations are provided separately via DefinitelyTyped as @types/sinon (e.g., version 21.0.0). [2]
Answer: install both:
sinon@types/sinon[2]
Sources
[1] (app.unpkg.com)
[2] (npmjs.com)
Citations:
- 1: https://app.unpkg.com/sinon%4021.0.0/files/package.json?utm_source=openai
- 2: https://www.npmjs.com/package/%40types/sinon?utm_source=openai
Add @types/sinon to devDependencies.
The sinon package (21.0.1) does not include bundled TypeScript type definitions. Since the project uses TypeScript with sinon in tests, @types/sinon must be explicitly added to devDependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-converters/package.json` at line 40, The package.json
devDependencies are missing TypeScript typings for sinon; add `@types/sinon` to
devDependencies so tests using sinon in TypeScript compile. Update
packages/kg-converters package.json by adding an entry for "@types/sinon" under
devDependencies (choose a compatible version for sinon 21.x or use a caret
range) and run install to update lockfiles; ensure imports in tests continue to
reference sinon as before.
6cc6497 to
294a52e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1761 +/- ##
==========================================
- Coverage 88.51% 83.81% -4.71%
==========================================
Files 201 199 -2
Lines 18456 13039 -5417
Branches 2019 1930 -89
==========================================
- Hits 16337 10929 -5408
+ Misses 2108 2098 -10
- Partials 11 12 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
packages/kg-converters/src/mobiledoc-to-lexical.ts (3)
142-145:⚠️ Potential issue | 🟠 MajorHandle parsed
nullbefore reading.sections.Line 144 can throw for input
"null"becauseJSON.parsereturnsnull.Proposed fix
- const mobiledoc = JSON.parse(serializedMobiledoc) as MobiledocDocument; - - if (!mobiledoc.sections) { + const mobiledoc = JSON.parse(serializedMobiledoc) as MobiledocDocument | null; + + if (!Array.isArray(mobiledoc?.sections)) { return JSON.stringify(BLANK_DOC); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/mobiledoc-to-lexical.ts` around lines 142 - 145, The code parses serializedMobiledoc into mobiledoc then reads mobiledoc.sections, which will throw if JSON.parse returns null (e.g., input "null"); update the guard to check for a null/undefined mobiledoc before accessing .sections (e.g., replace the existing if (!mobiledoc.sections) check with a combined check like if (!mobiledoc || !mobiledoc.sections) and return JSON.stringify(BLANK_DOC)); ensure the change references the parsed variable mobiledoc and the existing BLANK_DOC return path so the function safely handles serialized "null".
246-257:⚠️ Potential issue | 🟠 MajorReset link attributes when opening/closing
<a>markups.
relis not cleared, so a later link withoutrelcan inherit stale data.Proposed fix
openMarkupsIndexes.forEach((markupIndex: number) => { const markup = markups[markupIndex]; // Extract the href from the markup if it's a link if (markup[0] === 'a') { openLinkMarkup = true; + href = undefined; + rel = undefined; const attrs = markup[1] as unknown[] | undefined; if (attrs && attrs[0] === 'href') { href = attrs[1] as string; @@ if (markup && markup[0] === 'a') { embedChildNode(lexicalNode, linkNode); openLinkMarkup = false; href = undefined; + rel = undefined; linkNode = undefined; }Also applies to: 291-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/mobiledoc-to-lexical.ts` around lines 246 - 257, The link state (openLinkMarkup, href, rel) is not being reset, so rel can leak to subsequent links; inside the openMarkupsIndexes.forEach loop in mobiledoc-to-lexical.ts (the block that checks if markup[0] === 'a') explicitly clear or reset href and rel when attrs are missing and when closing an <a> markup: set href = undefined and rel = undefined by default before reading attrs, assign them only if present, and when you detect a closing link (where you set openLinkMarkup = false) also clear href and rel; apply the same fix in the identical code path around lines 291–295.
128-130:⚠️ Potential issue | 🟠 MajorGuard
backgroundColortype before regex check.Line 129 can throw when
payload.backgroundColoris truthy but not a string.Proposed fix
- if (payload.backgroundColor && !(payload.backgroundColor as string).match(/^[a-zA-Z\d-]+$/)) { + if ( + payload.backgroundColor !== undefined && + (typeof payload.backgroundColor !== 'string' || !/^[a-zA-Z\d-]+$/.test(payload.backgroundColor)) + ) { payload.backgroundColor = 'white'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/mobiledoc-to-lexical.ts` around lines 128 - 130, In the callout handler (callout: (payload: Record<string, unknown>) => { ... }) you currently run a string regex on payload.backgroundColor which can throw if the value is truthy but not a string; guard the check by verifying typeof payload.backgroundColor === 'string' before calling .match(), and if it is not a string (or the match fails) set payload.backgroundColor = 'white' to normalize the value.packages/kg-converters/src/lexical-to-mobiledoc.ts (2)
301-303:⚠️ Potential issue | 🟠 MajorPreserve link
relwhen serializing Mobiledoc markups.Current serialization drops
rel, so link metadata is lost on round-trip.Proposed fix
- const linkMarkup: MobiledocMarkup = ['a', ['href', child.url]]; + const attrs: unknown[] = ['href', child.url]; + if (typeof child.rel === 'string' && child.rel) { + attrs.push('rel', child.rel); + } + const linkMarkup: MobiledocMarkup = ['a', attrs];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/lexical-to-mobiledoc.ts` around lines 301 - 303, The current serialization for link nodes only pushes ['a', ['href', child.url]] into mobiledoc.markups and drops link metadata like rel; update the link markup creation in lexical-to-mobiledoc.ts (the block handling child.type === 'link') to include the rel attribute when present (i.e., construct markup entries for ['href', child.url] and, if child.rel exists, also ['rel', child.rel] before pushing to mobiledoc.markups so linkMarkup preserves child.rel on round-trip).
117-120:⚠️ Potential issue | 🟠 MajorGuard parsed lexical value before reading
.root.Line 119 can throw when input is
"null".Proposed fix
- const lexical = JSON.parse(serializedLexical) as {root?: {children?: LexicalNode[]}}; - - if (!lexical.root) { + const lexical = JSON.parse(serializedLexical) as {root?: {children?: LexicalNode[]}} | null; + + if (!lexical?.root) { return JSON.stringify(BLANK_DOC); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/lexical-to-mobiledoc.ts` around lines 117 - 120, The code reads lexical.root without guarding against the parsed value being null (e.g., when serializedLexical === "null"); update the guard to check the parsed object before accessing .root by replacing the current if (!lexical.root) { ... } with a null-safe check such as if (!lexical || !lexical.root) { return JSON.stringify(BLANK_DOC); }, referencing the parsed variable lexical, the input serializedLexical, and BLANK_DOC to locate the spot.packages/kg-converters/package.json (1)
10-16:⚠️ Potential issue | 🟠 MajorReorder conditional exports to avoid ESM resolution being shadowed.
"node"is currently before"import"/"require". In Node conditional exports, that can route ESM imports to CJS unexpectedly.Proposed fix
"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/esm/index.js" } },In Node.js conditional exports, does condition key order determine precedence? For {"node":"...","import":"...","require":"..."}, which target is selected for an ESM import?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/package.json` around lines 10 - 16, The conditional export ordering in package.json's "exports" object is wrong: the "node" condition appears before "import"/"require" which can cause ESM resolution to be shadowed; update the "exports" mapping so that "import" and "require" appear before "node" (i.e., ensure the "exports" keys are ordered "types"/"." metadata then "import", "require", and finally "node") so ESM imports resolve to the ESM build; adjust the "exports" block accordingly in package.json referencing the existing "exports" object and the "node", "import", and "require" keys.
🤖 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-converters/package.json`:
- Around line 20-23: Replace the duplicated, non-portable echo call with a
single cross-platform npm script and reference it from build, prepare, and
pretest: add a new script (e.g., "write-esm-package") that uses Node
(fs.mkdirSync with {recursive:true} and fs.writeFileSync) to create build/esm
and write {"type":"module"} to build/esm/package.json, then simplify "build",
"prepare", and "pretest" to run their tsc commands and invoke the new
"write-esm-package" script instead of repeating the echo snippet.
---
Duplicate comments:
In `@packages/kg-converters/package.json`:
- Around line 10-16: The conditional export ordering in package.json's "exports"
object is wrong: the "node" condition appears before "import"/"require" which
can cause ESM resolution to be shadowed; update the "exports" mapping so that
"import" and "require" appear before "node" (i.e., ensure the "exports" keys are
ordered "types"/"." metadata then "import", "require", and finally "node") so
ESM imports resolve to the ESM build; adjust the "exports" block accordingly in
package.json referencing the existing "exports" object and the "node", "import",
and "require" keys.
In `@packages/kg-converters/src/lexical-to-mobiledoc.ts`:
- Around line 301-303: The current serialization for link nodes only pushes
['a', ['href', child.url]] into mobiledoc.markups and drops link metadata like
rel; update the link markup creation in lexical-to-mobiledoc.ts (the block
handling child.type === 'link') to include the rel attribute when present (i.e.,
construct markup entries for ['href', child.url] and, if child.rel exists, also
['rel', child.rel] before pushing to mobiledoc.markups so linkMarkup preserves
child.rel on round-trip).
- Around line 117-120: The code reads lexical.root without guarding against the
parsed value being null (e.g., when serializedLexical === "null"); update the
guard to check the parsed object before accessing .root by replacing the current
if (!lexical.root) { ... } with a null-safe check such as if (!lexical ||
!lexical.root) { return JSON.stringify(BLANK_DOC); }, referencing the parsed
variable lexical, the input serializedLexical, and BLANK_DOC to locate the spot.
In `@packages/kg-converters/src/mobiledoc-to-lexical.ts`:
- Around line 142-145: The code parses serializedMobiledoc into mobiledoc then
reads mobiledoc.sections, which will throw if JSON.parse returns null (e.g.,
input "null"); update the guard to check for a null/undefined mobiledoc before
accessing .sections (e.g., replace the existing if (!mobiledoc.sections) check
with a combined check like if (!mobiledoc || !mobiledoc.sections) and return
JSON.stringify(BLANK_DOC)); ensure the change references the parsed variable
mobiledoc and the existing BLANK_DOC return path so the function safely handles
serialized "null".
- Around line 246-257: The link state (openLinkMarkup, href, rel) is not being
reset, so rel can leak to subsequent links; inside the
openMarkupsIndexes.forEach loop in mobiledoc-to-lexical.ts (the block that
checks if markup[0] === 'a') explicitly clear or reset href and rel when attrs
are missing and when closing an <a> markup: set href = undefined and rel =
undefined by default before reading attrs, assign them only if present, and when
you detect a closing link (where you set openLinkMarkup = false) also clear href
and rel; apply the same fix in the identical code path around lines 291–295.
- Around line 128-130: In the callout handler (callout: (payload: Record<string,
unknown>) => { ... }) you currently run a string regex on
payload.backgroundColor which can throw if the value is truthy but not a string;
guard the check by verifying typeof payload.backgroundColor === 'string' before
calling .match(), and if it is not a string (or the match fails) set
payload.backgroundColor = 'white' to normalize the value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e61386ce-4bbc-42ed-9c56-84dba59c4584
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
packages/kg-converters/.gitignorepackages/kg-converters/eslint.config.mjspackages/kg-converters/index.jspackages/kg-converters/lib/kg-converters.jspackages/kg-converters/package.jsonpackages/kg-converters/rollup.config.mjspackages/kg-converters/src/index.tspackages/kg-converters/src/kg-converters.tspackages/kg-converters/src/lexical-to-mobiledoc.tspackages/kg-converters/src/mobiledoc-to-lexical.tspackages/kg-converters/test/exports.test.jspackages/kg-converters/test/exports.test.tspackages/kg-converters/test/lexical-to-mobiledoc.test.tspackages/kg-converters/test/mobiledoc-to-lexical.test.tspackages/kg-converters/tsconfig.cjs.jsonpackages/kg-converters/tsconfig.json
💤 Files with no reviewable changes (4)
- packages/kg-converters/lib/kg-converters.js
- packages/kg-converters/rollup.config.mjs
- packages/kg-converters/test/exports.test.js
- packages/kg-converters/index.js
✅ Files skipped from review due to trivial changes (1)
- packages/kg-converters/tsconfig.cjs.json
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/kg-converters/test/lexical-to-mobiledoc.test.ts
- packages/kg-converters/test/exports.test.ts
- packages/kg-converters/src/index.ts
- packages/kg-converters/.gitignore
- packages/kg-converters/test/mobiledoc-to-lexical.test.ts
- packages/kg-converters/tsconfig.json
Comment on lines
+20
to
+23
| "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", | ||
| "test:unit": "NODE_ENV=testing c8 --all --src src/ --check-coverage --reporter text --reporter cobertura tsx node_modules/.bin/mocha './test/**/*.test.ts'", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In npm scripts, is echo '{"type":"module"}' > file with single quotes portable across POSIX shells and Windows cmd/PowerShell?
💡 Result:
No.
- POSIX shells (
/bin/sh,bash,dash, etc.):echo '{"type":"module"}' > fileis fine—single quotes are standard shell quoting. npm runs scripts via/bin/shby default on Unix-like systems. [1] - Windows (default npm behavior): npm runs scripts via
cmd.exeby default (even if you launchnpm runfrom PowerShell), andcmd.exedoes not treat'...'as quoting—single quotes are just literal characters. So you’ll typically end up writing the quotes into the file (or otherwise get different parsing than you expect). [1][2] - PowerShell: PowerShell does support single-quoted strings, so the command itself would work if npm were using PowerShell as the script shell—but by default it isn’t. You can change the shell with npm’s
script-shellconfig. [1][3]
If you need cross-platform behavior, prefer a Node-based write (e.g., node -e "require('fs').writeFileSync('file','{\"type\":\"module\"}\\n')"), or explicitly set script-shell to pwsh / bash for your project/CI. [3]
Sources
[1] npm Docs — npm run-script: default shell is /bin/sh on Unix, cmd.exe on Windows https://docs.npmjs.com/cli/v10/commands/npm-run-script/
[2] Stack Overflow — cmd.exe does not recognize single quotes as string delimiters https://stackoverflow.com/questions/562038/escaping-double-quotes-in-batch-script
[3] npm Docs — script-shell config (default /bin/sh on POSIX, cmd.exe on Windows) https://docs.npmjs.com/cli/v10/using-npm/config/
🏁 Script executed:
# Verify the file exists and check its content
if [ -f "packages/kg-converters/package.json" ]; then
echo "=== File content (full) ==="
cat -n "packages/kg-converters/package.json"
echo ""
echo "=== Lines 15-30 (context around lines 20-23) ==="
sed -n '15,30p' "packages/kg-converters/package.json"
else
echo "File not found at packages/kg-converters/package.json"
# Try to find it
find . -name "package.json" -path "*/kg-converters/*" 2>/dev/null | head -5
fiRepository: TryGhost/Koenig
Length of output: 2723
Fix shell portability and eliminate build script duplication.
The echo '{"type":"module"}' > build/esm/package.json pattern is not portable to Windows (cmd.exe does not recognize single quotes as string delimiters), and the same 71-character build command is duplicated across build, prepare, and pretest.
Proposed fix
"scripts": {
"dev": "tsc --watch --preserveWatchOutput",
- "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",
+ "write:esm-package-json": "node -e \"require('node:fs').writeFileSync('build/esm/package.json', JSON.stringify({type:'module'}))\"",
+ "build": "tsc && tsc -p tsconfig.cjs.json && yarn write:esm-package-json",
+ "prepare": "yarn build",
+ "pretest": "yarn build",
"test:unit": "NODE_ENV=testing c8 --all --src src/ --check-coverage --reporter text --reporter cobertura tsx node_modules/.bin/mocha './test/**/*.test.ts'",📝 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
| "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", | |
| "test:unit": "NODE_ENV=testing c8 --all --src src/ --check-coverage --reporter text --reporter cobertura tsx node_modules/.bin/mocha './test/**/*.test.ts'", | |
| "dev": "tsc --watch --preserveWatchOutput", | |
| "write:esm-package-json": "node -e \"require('node:fs').writeFileSync('build/esm/package.json', JSON.stringify({type:'module'}))\"", | |
| "build": "tsc && tsc -p tsconfig.cjs.json && yarn write:esm-package-json", | |
| "prepare": "yarn build", | |
| "pretest": "yarn build", | |
| "test:unit": "NODE_ENV=testing c8 --all --src src/ --check-coverage --reporter text --reporter cobertura tsx node_modules/.bin/mocha './test/**/*.test.ts'", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-converters/package.json` around lines 20 - 23, Replace the
duplicated, non-portable echo call with a single cross-platform npm script and
reference it from build, prepare, and pretest: add a new script (e.g.,
"write-esm-package") that uses Node (fs.mkdirSync with {recursive:true} and
fs.writeFileSync) to create build/esm and write {"type":"module"} to
build/esm/package.json, then simplify "build", "prepare", and "pretest" to run
their tsc commands and invoke the new "write-esm-package" script instead of
repeating the echo snippet.
294a52e to
45d7c1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/kg-converters/src/mobiledoc-to-lexical.ts (3)
127-135:⚠️ Potential issue | 🟠 Major
backgroundColortype not validated before.match()call.The past review noted that a truthy non-string value (e.g.,
123) will throw when.match()is called. The castas stringdoesn't validate at runtime.Suggested fix
- callout: (payload: Record<string, unknown>) => { - if (payload.backgroundColor && !(payload.backgroundColor as string).match(/^[a-zA-Z\d-]+$/)) { + callout: (payload: Record<string, unknown>) => { + if ( + payload.backgroundColor !== undefined && + (typeof payload.backgroundColor !== 'string' || !/^[a-zA-Z\d-]+$/.test(payload.backgroundColor)) + ) { payload.backgroundColor = 'white'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/mobiledoc-to-lexical.ts` around lines 127 - 135, The callout card fixer in CARD_FIXES_MAP currently assumes payload.backgroundColor is a string and uses .match(), which can throw for non-string truthy values; update the callout function to first check typeof payload.backgroundColor === 'string' (or convert safely) before calling .match(), and only replace payload.backgroundColor with 'white' when the value is a string that fails the /^[a-zA-Z\d-]+$/ test (or when it's not a string and you want to default it), ensuring payload is returned unchanged for valid string values otherwise.
246-262:⚠️ Potential issue | 🟠 Major
relnot reset when link closes, causing state bleed to subsequent links.The past review noted that
hrefandlinkNodeare reset when a link closes (lines 294-295), butrelis not. If one link hasrel="noopener"and the next link has norel, the second link incorrectly inherits the first'srelvalue.Suggested fix
if (markup && markup[0] === 'a') { embedChildNode(lexicalNode, linkNode); openLinkMarkup = false; href = undefined; + rel = undefined; linkNode = undefined; }Also applies to: 291-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/mobiledoc-to-lexical.ts` around lines 246 - 262, The rel value from link markups isn't being cleared when a link closes, causing rel to persist across links; update the link-closing logic (where href and linkNode are reset) to also reset rel to undefined/empty so rel doesn't bleed into subsequent links. Locate the handling of openLinkMarkup / href / linkNode resets (the code that processes openMarkupsIndexes and the block that clears href and linkNode when closing a link) and add resetting of the rel variable there, and ensure rel is similarly initialized/cleared when opening a new 'a' markup in the openMarkupsIndexes loop that reads markups and attrs.
142-146:⚠️ Potential issue | 🟠 Major
JSON.parse('null')returnsnull, causing.sectionsaccess to throw.The past review flagged this same pattern. If
serializedMobiledocis the string"null",JSON.parsereturns the literalnull, and accessing.sectionsthrows.Suggested fix
- const mobiledoc = JSON.parse(serializedMobiledoc) as MobiledocDocument; + const mobiledoc = JSON.parse(serializedMobiledoc) as MobiledocDocument | null; - if (!mobiledoc.sections) { + if (!mobiledoc?.sections) { return JSON.stringify(BLANK_DOC); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/mobiledoc-to-lexical.ts` around lines 142 - 146, The code parses serializedMobiledoc into mobiledoc and then directly reads mobiledoc.sections, which will throw if JSON.parse returns null (e.g., serializedMobiledoc === "null"); update the logic in the mobiledoc-to-lexical converter to guard the parsed value (the mobiledoc variable) before accessing .sections — e.g., treat a falsy or non-object parsed result as an empty document and return JSON.stringify(BLANK_DOC) if mobiledoc is null/undefined or doesn't have sections; ensure the check references serializedMobiledoc, mobiledoc, and BLANK_DOC so the fix is applied around the existing parse and the immediate conditional.packages/kg-converters/src/lexical-to-mobiledoc.ts (2)
117-121:⚠️ Potential issue | 🟠 Major
JSON.parse('null')still causes a runtime error.The past review flagged that
JSON.parse('null')returns the literalnull, so accessinglexical.rooton line 119 will throw. The type cast on line 117 doesn't include| null, and there's no null check before accessing.root.Suggested fix
- const lexical = JSON.parse(serializedLexical) as {root?: {children?: LexicalNode[]}}; + const lexical = JSON.parse(serializedLexical) as {root?: {children?: LexicalNode[]}} | null; - if (!lexical.root) { + if (!lexical?.root) { return JSON.stringify(BLANK_DOC); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/lexical-to-mobiledoc.ts` around lines 117 - 121, The code assumes JSON.parse(serializedLexical) returns an object but JSON.parse('null') yields null, so accessing lexical.root will throw; update the parsing and guard: parse serializedLexical into a value typed as {root?: {children?: LexicalNode[]}} | null (or use a fallback like const lexical = JSON.parse(serializedLexical) ?? null) and add a null check before accessing lexical.root (e.g., if (!lexical || !lexical.root) return JSON.stringify(BLANK_DOC)); ensure references to lexical, serializedLexical and BLANK_DOC are used so the function safely handles 'null' input.
301-303:⚠️ Potential issue | 🟠 MajorLink
relattribute not preserved in Mobiledoc markup.The past review noted that only
hrefis serialized, causingrelto be lost on round-trip. The correspondingmobiledoc-to-lexical.tsreadsrel(lines 256-258), but this direction doesn't write it.Suggested fix
- const linkMarkup: MobiledocMarkup = ['a', ['href', child.url]]; + const attrs: unknown[] = ['href', child.url]; + if (typeof child.rel === 'string' && child.rel) { + attrs.push('rel', child.rel); + } + const linkMarkup: MobiledocMarkup = ['a', attrs];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-converters/src/lexical-to-mobiledoc.ts` around lines 301 - 303, The current Lexical->Mobiledoc conversion for link nodes only serializes the href, dropping the rel attribute; update the link serialization in lexical-to-mobiledoc.ts (the block handling child.type === 'link' where linkMarkup is created and pushed to mobiledoc.markups) to include rel when present (e.g., add a ['rel', child.rel] pair alongside ['href', child.url] so the markup contains both attributes before pushing), ensuring the produced Mobiledoc preserves rel for round-tripping with mobiledoc-to-lexical.ts.
🤖 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-converters/tsconfig.test.json`:
- Line 5: Remove the invalid outDir: null entry from compilerOptions in the
tsconfig used for tests; locate the tsconfig JSON that contains
"compilerOptions" and the "outDir" property (currently set to null) and delete
that outDir line — keep the existing "noEmit": true setting so no output is
produced.
---
Duplicate comments:
In `@packages/kg-converters/src/lexical-to-mobiledoc.ts`:
- Around line 117-121: The code assumes JSON.parse(serializedLexical) returns an
object but JSON.parse('null') yields null, so accessing lexical.root will throw;
update the parsing and guard: parse serializedLexical into a value typed as
{root?: {children?: LexicalNode[]}} | null (or use a fallback like const lexical
= JSON.parse(serializedLexical) ?? null) and add a null check before accessing
lexical.root (e.g., if (!lexical || !lexical.root) return
JSON.stringify(BLANK_DOC)); ensure references to lexical, serializedLexical and
BLANK_DOC are used so the function safely handles 'null' input.
- Around line 301-303: The current Lexical->Mobiledoc conversion for link nodes
only serializes the href, dropping the rel attribute; update the link
serialization in lexical-to-mobiledoc.ts (the block handling child.type ===
'link' where linkMarkup is created and pushed to mobiledoc.markups) to include
rel when present (e.g., add a ['rel', child.rel] pair alongside ['href',
child.url] so the markup contains both attributes before pushing), ensuring the
produced Mobiledoc preserves rel for round-tripping with
mobiledoc-to-lexical.ts.
In `@packages/kg-converters/src/mobiledoc-to-lexical.ts`:
- Around line 127-135: The callout card fixer in CARD_FIXES_MAP currently
assumes payload.backgroundColor is a string and uses .match(), which can throw
for non-string truthy values; update the callout function to first check typeof
payload.backgroundColor === 'string' (or convert safely) before calling
.match(), and only replace payload.backgroundColor with 'white' when the value
is a string that fails the /^[a-zA-Z\d-]+$/ test (or when it's not a string and
you want to default it), ensuring payload is returned unchanged for valid string
values otherwise.
- Around line 246-262: The rel value from link markups isn't being cleared when
a link closes, causing rel to persist across links; update the link-closing
logic (where href and linkNode are reset) to also reset rel to undefined/empty
so rel doesn't bleed into subsequent links. Locate the handling of
openLinkMarkup / href / linkNode resets (the code that processes
openMarkupsIndexes and the block that clears href and linkNode when closing a
link) and add resetting of the rel variable there, and ensure rel is similarly
initialized/cleared when opening a new 'a' markup in the openMarkupsIndexes loop
that reads markups and attrs.
- Around line 142-146: The code parses serializedMobiledoc into mobiledoc and
then directly reads mobiledoc.sections, which will throw if JSON.parse returns
null (e.g., serializedMobiledoc === "null"); update the logic in the
mobiledoc-to-lexical converter to guard the parsed value (the mobiledoc
variable) before accessing .sections — e.g., treat a falsy or non-object parsed
result as an empty document and return JSON.stringify(BLANK_DOC) if mobiledoc is
null/undefined or doesn't have sections; ensure the check references
serializedMobiledoc, mobiledoc, and BLANK_DOC so the fix is applied around the
existing parse and the immediate conditional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b52de99-d666-44ff-8c47-91d066b21fee
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
packages/kg-converters/.gitignorepackages/kg-converters/eslint.config.mjspackages/kg-converters/index.jspackages/kg-converters/package.jsonpackages/kg-converters/rollup.config.mjspackages/kg-converters/src/index.tspackages/kg-converters/src/kg-converters.tspackages/kg-converters/src/lexical-to-mobiledoc.tspackages/kg-converters/src/mobiledoc-to-lexical.tspackages/kg-converters/test/exports.test.tspackages/kg-converters/test/lexical-to-mobiledoc.test.tspackages/kg-converters/test/mobiledoc-to-lexical.test.tspackages/kg-converters/tsconfig.cjs.jsonpackages/kg-converters/tsconfig.jsonpackages/kg-converters/tsconfig.test.json
💤 Files with no reviewable changes (2)
- packages/kg-converters/rollup.config.mjs
- packages/kg-converters/index.js
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/kg-converters/tsconfig.cjs.json
- packages/kg-converters/.gitignore
- packages/kg-converters/test/mobiledoc-to-lexical.test.ts
- packages/kg-converters/test/exports.test.ts
- packages/kg-converters/src/kg-converters.ts
| "extends": "./tsconfig.json", | ||
| "compilerOptions": { | ||
| "rootDir": ".", | ||
| "outDir": null, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TypeScript tsconfig outDir null valid value
💡 Result:
No. compilerOptions.outDir is not a nullable option—TypeScript expects a path string when outDir is present. If you don’t want an output directory, omit outDir entirely; then .js files are emitted next to their corresponding .ts sources. [1]
If your goal is “don’t emit anything”, use "noEmit": true instead. [2]
Sources:
[1] TypeScript TSConfig option outDir docs https://www.typescriptlang.org/tsconfig/outDir.html
[2] TypeScript TSConfig reference (noEmit) https://www.typescriptlang.org/tsconfig/
Remove outDir: null from compilerOptions.
TypeScript's outDir expects a string path and does not support null as a value. Since noEmit: true is already configured (which prevents any output), this setting is both invalid and redundant.
Suggested fix
"compilerOptions": {
"rootDir": ".",
- "outDir": null,
"noEmit": true,📝 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
| "outDir": null, | |
| "compilerOptions": { | |
| "rootDir": ".", | |
| "noEmit": true, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-converters/tsconfig.test.json` at line 5, Remove the invalid
outDir: null entry from compilerOptions in the tsconfig used for tests; locate
the tsconfig JSON that contains "compilerOptions" and the "outDir" property
(currently set to null) and delete that outDir line — keep the existing
"noEmit": true setting so no output is produced.
45d7c1d to
d597944
Compare
- Remove Rollup build (rollup.config.mjs, @rollup/plugin-babel) - Move lib/ to src/, rename .js to .ts - Add tsconfig.json (strict, NodeNext, ESM) - Add "type": "module" to package.json - Convert source and tests to ESM with type annotations - Replace .eslintrc.js with eslint.config.js (flat config) - Output to build/ via tsc (replaces cjs/ and es/ dirs)
kevinansfield
force-pushed
the
ts/kg-converters
branch
from
d597944 to
7ab3f00
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-convertersyarn lintpasses in kg-converters