Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/generators/ast/generate.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export async function processChunk(inputSlice, itemIndices) {
tree: remark().parse(value),
// The path is the relative path minus the extension
path: relativePath,
fullPath: path,
});
}

Expand Down
22 changes: 21 additions & 1 deletion src/generators/jsx-ast/utils/__tests__/buildContent.test.mjs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import assert from 'node:assert/strict';
import { resolve } from 'node:path';
import { describe, it } from 'node:test';

import { setConfig } from '../../../../utils/configuration/index.mjs';
import { transformHeadingNode } from '../buildContent.mjs';
import buildContent, { transformHeadingNode } from '../buildContent.mjs';

const heading = {
type: 'heading',
Expand Down Expand Up @@ -66,3 +67,22 @@ describe('transformHeadingNode (deprecation Type -> AlertBox level)', () => {
assert.equal(levelAttr.value, 'danger');
});
});

describe('Asset Extraction and URL Rewriting', () => {
it('should rewrite local image URLs and populate the assetsMap', async () => {
const mockEntries = [
{
fullPath: '/node/doc/api/fs.md',
content: {
type: 'root',
children: [{ type: 'image', url: './logo.png' }],
},
},
];

const result = await buildContent(mockEntries, { api: 'test' });

const expectedSource = resolve('/node/doc/api', './logo.png');
assert.strictEqual(result.assetsMap[expectedSource], 'logo.png');
});
});
60 changes: 59 additions & 1 deletion src/generators/jsx-ast/utils/buildContent.mjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';

import { resolve, dirname, basename } from 'node:path';

import { h as createElement } from 'hastscript';
import { slice } from 'mdast-util-slice-markdown';
import readingTime from 'reading-time';
Expand Down Expand Up @@ -287,6 +289,59 @@ export const createDocumentLayout = (entries, metadata) =>
}),
]);

/**
* Checks if a given URL is a local asset path.
*
* @param {string} url - The URL or path to check.
* @returns {boolean} True if the asset is local, false otherwise.
*/
function isLocalAsset(url) {
if (!url || url.startsWith('//')) {
return false;
}

try {
new URL(url);
return false;
} catch {
return true;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use URL.parse() instead of new URL()

Low Severity

isLocalAsset uses new URL(url) inside a try/catch to detect local paths. The project convention prefers URL.parse(), which returns null for invalid URLs instead of throwing. This can be simplified to return URL.parse(url) === null.

Fix in Cursor Fix in Web

Triggered by learned rule: Prefer URL.parse() and destructuring over new URL().property

Reviewed by Cursor Bugbot for commit 536ed59. Configure here.

}

/**
* Traverses the AST of markdown files to find local image references,
* rewrites their URLs to a relative assets folder, and collects their paths.
*
* @param {Array<import('../../metadata/types').MetadataEntry>} metadataEntries - API documentation metadata entries
* @returns {Map<string, string>} A Map containing source paths as keys and destination paths as values.
*/
function extractAssetsFromAST(metadataEntries) {
const assetsMap = new Map();
for (const entry of metadataEntries) {
if (!entry.content) {
continue;
}
visit(entry.content, 'image', imageNode => {
const originalUrl = imageNode.url;

if (isLocalAsset(originalUrl)) {
const sourceDir = entry.fullPath
? dirname(entry.fullPath)
: process.cwd();
const sourcePath = resolve(sourceDir, originalUrl);
const fileName = basename(originalUrl);

assetsMap.set(sourcePath, fileName);

// Rewrite AST URL
imageNode.url = `/assets/${fileName}`;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded 'assets' path duplicated across two generators

Low Severity

The string 'assets' is hardcoded as an output directory name in both buildContent.mjs (URL rewrite to `/assets/${fileName}`) and generate.mjs (filesystem path via join(outputDir, 'assets', name)). Since this is a cross-generator constant controlling output path structure, it belongs as a named constant. Changing it in one generator without the other would silently break image resolution.

Additional Locations (1)
Fix in Cursor Fix in Web

Triggered by learned rule: Regex patterns and external URLs belong in constants files

Reviewed by Cursor Bugbot for commit 536ed59. Configure here.

}
});
}

return assetsMap;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asset filename collisions silently overwrite different images

Medium Severity

extractAssetsFromAST uses basename(originalUrl) as the destination filename, so two different images with the same name from different directories (e.g., ./diagrams/arch.png in fs.md and ./diagrams/arch.png in net.md) both map to arch.png in /assets/. One copy silently overwrites the other, and both markdown files end up referencing the wrong image. The AST URLs are also both rewritten to /assets/arch.png, making the collision unrecoverable.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 536ed59. Configure here.


/**
* @typedef {import('estree').Node & { data: import('../../metadata/types').MetadataEntry }} JSXContent
*
Expand All @@ -296,6 +351,9 @@ export const createDocumentLayout = (entries, metadata) =>
* @returns {Promise<JSXContent>}
*/
const buildContent = async (metadataEntries, head) => {
// First extract assets and rewrite URLs in the AST
const assetsMap = Object.fromEntries(extractAssetsFromAST(metadataEntries));

// The metadata is the heading without the node children
const metadata = omitKeys(head, [
'content',
Expand All @@ -311,7 +369,7 @@ const buildContent = async (metadataEntries, head) => {
const ast = await remark().run(root);

// The final MDX content is the expression in the Program's first body node
return { ...ast.body[0].expression, data: head };
return { ...ast.body[0].expression, data: head, assetsMap };
};

export default buildContent;
3 changes: 2 additions & 1 deletion src/generators/metadata/utils/parse.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import { IGNORE_STABILITY_STEMS } from '../constants.mjs';
* @param {Record<string, string>} typeMap
* @returns {Promise<Array<import('../types').MetadataEntry>>}
*/
export const parseApiDoc = ({ path, tree }, typeMap) => {
export const parseApiDoc = ({ path, tree, fullPath }, typeMap) => {
/**
* Collection of metadata entries for the file
* @type {Array<import('../types').MetadataEntry>}
Expand Down Expand Up @@ -85,6 +85,7 @@ export const parseApiDoc = ({ path, tree }, typeMap) => {
path,
basename: basename(path),
heading: headingNode,
fullPath,
});

// Generate slug and update heading data
Expand Down
38 changes: 37 additions & 1 deletion src/generators/web/generate.mjs
Original file line number Diff line number Diff line change
@@ -1,12 +1,46 @@
'use strict';

import { readFile } from 'node:fs/promises';
import { readFile, cp, mkdir } from 'node:fs/promises';
import { join } from 'node:path';

import { processJSXEntries } from './utils/processing.mjs';
import getConfig from '../../utils/configuration/index.mjs';
import { writeFile } from '../../utils/file.mjs';

/**
* Aggregates and copies local assets referenced in the markdown ASTs to the output directory.
*
* @param {Array<import('./types').JSXContent>} input - The processed entries containing asset maps
* @param {string} outputDir - The absolute path to the generation output directory
* @returns {Promise<void>}
*/
async function copyProjectAssets(input, outputDir) {
const allAssets = new Map();

for (const entry of input) {
if (entry.assetsMap) {
for (const [source, name] of Object.entries(entry.assetsMap)) {
allAssets.set(source, join(outputDir, 'assets', name));
}
}
}

if (allAssets.size === 0) {
return;
}

const assetsOutputDir = join(outputDir, 'assets');
await mkdir(assetsOutputDir, { recursive: true });

for (const [source, dest] of allAssets.entries()) {
try {
await cp(source, dest, { force: true });
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use copyFile with COPYFILE_FICLONE for files

Low Severity

cp is used to copy individual asset files, but the project convention requires copyFile from node:fs/promises with the constants.COPYFILE_FICLONE flag for single-file copies. copyFile with COPYFILE_FICLONE attempts a zero-copy clone and gracefully falls back, which is more efficient for single files than cp.

Fix in Cursor Fix in Web

Triggered by learned rule: Use copyFile with COPYFILE_FICLONE instead of manual readFile+writeFile

Reviewed by Cursor Bugbot for commit 536ed59. Configure here.

} catch (err) {
console.error(`[doc-kit] Error copying asset: ${source}`, err);
}
}
}

/**
* Main generation function that processes JSX AST entries into web bundles.
*
Expand Down Expand Up @@ -34,6 +68,8 @@ export async function generate(input) {

// Write CSS bundle
await writeFile(join(config.output, 'styles.css'), css, 'utf-8');

await copyProjectAssets(input, config.output);
}

return results.map(({ html }) => ({ html: html.toString(), css }));
Expand Down
Loading