fix(cli): don't rewrite @/ imports inside MDX code blocks - #17406
fix(cli): don't rewrite @/ imports inside MDX code blocks#17406fern-api[bot] wants to merge 1 commit into
Conversation
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Solid targeted fix: parse the page, collect code/inlineCode ranges, skip regex matches that land inside them. Main concern is that grayMatter() is called outside the try/catch, so a page with malformed YAML frontmatter would now throw where it previously didn't — a publish-path regression. Minor readability nit on the visit test predicate.
- 🟡 1 warning(s)
- 🔵 2 suggestion(s)
| const { content } = grayMatter(markdown); | ||
| // `parseMarkdownToTree` strips frontmatter, so node offsets are relative to the body. | ||
| if (!markdown.endsWith(content)) { | ||
| return undefined; | ||
| } | ||
| const bodyOffset = markdown.length - content.length; | ||
|
|
||
| try { | ||
| const tree = parseMarkdownToTree(markdown); |
There was a problem hiding this comment.
🟡 warning
grayMatter() sits outside the try, so malformed YAML frontmatter now throws out of transformAtPrefixImports — a path that previously never parsed anything and couldn't fail. Given this runs on publish (DocsDefinitionResolver, publishDocs), a broken frontmatter block would go from "imports rewritten anyway" to "build blows up". Move it inside the guard.
| const { content } = grayMatter(markdown); | |
| // `parseMarkdownToTree` strips frontmatter, so node offsets are relative to the body. | |
| if (!markdown.endsWith(content)) { | |
| return undefined; | |
| } | |
| const bodyOffset = markdown.length - content.length; | |
| try { | |
| const tree = parseMarkdownToTree(markdown); | |
| try { | |
| const { content } = grayMatter(markdown); | |
| // `parseMarkdownToTree` strips frontmatter, so node offsets are relative to the body. | |
| if (!markdown.endsWith(content)) { | |
| return undefined; | |
| } | |
| const bodyOffset = markdown.length - content.length; | |
| const tree = parseMarkdownToTree(markdown); |
| visit( | ||
| tree, | ||
| (node: unknown) => { | ||
| const type = (node as { type?: string }).type; | ||
| return type === "code" || type === "inlineCode"; | ||
| }, |
There was a problem hiding this comment.
🔵 suggestion
unist-util-visit accepts a type array directly, which drops the unknown cast gymnastics:
| visit( | |
| tree, | |
| (node: unknown) => { | |
| const type = (node as { type?: string }).type; | |
| return type === "code" || type === "inlineCode"; | |
| }, | |
| visit( | |
| tree, | |
| ["code", "inlineCode"], |
| const bodyOffset = markdown.length - content.length; | ||
|
|
||
| try { | ||
| const tree = parseMarkdownToTree(markdown); |
There was a problem hiding this comment.
🔵 suggestion
bodyOffset silently depends on parseMarkdownToTree stripping frontmatter internally. If that ever changes to keep frontmatter as a node (offsets become absolute), every range shifts by the frontmatter length and code blocks get rewritten again — with no test failure signal beyond the one frontmatter test. Consider passing content explicitly (parseMarkdownToTree(content)) so the offset base is unambiguous at this call site.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| } catch { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
🟡 Parsing failures are swallowed with no logging
Errors from parsing the page are discarded by an empty catch block (catch {} at packages/cli/docs-markdown-utils/src/transformAtPrefixImports.ts:52-54), which the repository conventions forbid, so when a page fails to parse the code-block protection silently disappears with no diagnostic.
Impact: Users see code samples silently rewritten again on unparseable pages with no clue why.
Empty catch violates REVIEW.md TypeScript convention
REVIEW.md states: "No empty catch blocks -- at minimum log the error." Here getCodeRanges swallows any error thrown by parseMarkdownToTree (packages/cli/docs-markdown-utils/src/parseMarkdownToTree.ts:11-17), returns undefined, and the caller then falls back to rewriting every match, including matches inside fenced code. At minimum the error should be logged/surfaced.
Prompt for agents
REVIEW.md forbids empty catch blocks. In getCodeRanges (packages/cli/docs-markdown-utils/src/transformAtPrefixImports.ts), the catch around parseMarkdownToTree discards the error entirely. Either log the parse failure (console.warn or the package's task-context logger if one is available at the call site) or thread a logger/taskContext through so the silent fallback to the old, buggy rewrite behavior is observable.
Was this helpful? React with 👍 or 👎 to provide feedback.
| visit( | ||
| tree, | ||
| (node: unknown) => { | ||
| const type = (node as { type?: string }).type; | ||
| return type === "code" || type === "inlineCode"; | ||
| }, |
There was a problem hiding this comment.
🟡 Type assertion used where the repository forbids it
The node passed to the traversal check is force-cast to a shape (node as { type?: string } at packages/cli/docs-markdown-utils/src/transformAtPrefixImports.ts:38-40) instead of being typed, which the repository conventions disallow.
Impact: None at runtime; it breaks an explicit code convention of the project.
REVIEW.md: "No `as X` type assertions unless the compiler genuinely cannot infer the type"
The assertion is only needed because the predicate parameter is annotated unknown. unist-util-visit accepts a test as a string or array of node type strings, e.g. visit(tree, ["code", "inlineCode"], (node) => { ... }), which removes the need for both the unknown annotation and the cast, and matches how other code in the repo tests node types.
| visit( | |
| tree, | |
| (node: unknown) => { | |
| const type = (node as { type?: string }).type; | |
| return type === "code" || type === "inlineCode"; | |
| }, | |
| visit( | |
| tree, | |
| ["code", "inlineCode"], |
Was this helpful? React with 👍 or 👎 to provide feedback.
Description
transformAtPrefixImportsresolved@/-prefixed MDX imports with a singlemarkdown.replace()over the raw page, so any line matchingimport ... from "@/..."was rewritten to a path relative to the MDX file — including lines inside fenced code blocks and inline code. A customer documenting a Next.js app saw their code sample silently mutated: