From eb2cfcb8de82b53170e87ff5d3363dc59d838fe9 Mon Sep 17 00:00:00 2001 From: mehnoorsiddiqui Date: Thu, 22 Oct 2026 12:02:01 +0500 Subject: [PATCH] fix(portal): report content pages that are not valid MDX before the build or preview reaches them An `.mdx` page that is not valid MDX, such as an unclosed `` or a mismatched closing tag, is now refused by `portal generate` and `portal serve`, at startup and on save. Every broken page is listed in one run, by its path in `src`, line and column, in MDX's own words without the bundler's internals. MDX stops at the first mistake in a page, and the message says so. Closes #443 Co-Authored-By: Claude Opus 5.5 (1M context) --- src/prompts/portal/source.ts | 19 +++++++ src/types/portal/content-tree.ts | 10 ++++ src/types/portal/page.ts | 66 ++++++++++++++++++++---- src/types/portal/portal-source.ts | 9 +++- test/prompts/portal/source.test.ts | 35 +++++++++++++ test/types/portal-source-context.test.ts | 41 +++++++++++++++ test/types/portal/content-tree.test.ts | 29 +++++++++++ test/types/portal/page.test.ts | 48 ++++++++++++++++- 8 files changed, 246 insertions(+), 11 deletions(-) diff --git a/src/prompts/portal/source.ts b/src/prompts/portal/source.ts index b0136e51a..0973ef1ac 100644 --- a/src/prompts/portal/source.ts +++ b/src/prompts/portal/source.ts @@ -6,6 +6,7 @@ import { ContentNotices, NoTabsListed } from '../../types/portal/content-notices import { API_REFERENCE_TOKEN, NAVIGATION_FILE_NAME, TabEntry } from '../../types/portal/portal-navigation.js'; import { ContentProblem, + InvalidPage, MissingFile, MissingImage, PortalSourceProblem, @@ -327,9 +328,27 @@ function reportContentProblem(problem: ContentProblem, sourceDirectory: Director reportMissingImages(problem.images, sourceDirectory); return; } + case 'invalidMdx': { + reportInvalidPages(problem.pages, sourceDirectory); + return; + } } } +function reportInvalidPages(pages: InvalidPage[], sourceDirectory: DirectoryPath): void { + const lines = pages.map( + ({ page, line, column, reason }) => + ` • ${relative(page, sourceDirectory)}, line ${line}, column ${column}: ${reason}` + ); + log.error( + pages.length === 1 + ? `A page in ${f.path(sourceDirectory)} is not valid MDX:` + : `Pages in ${f.path(sourceDirectory)} are not valid MDX:` + ); + log.message(lines.join('\n')); + log.message('MDX stops at the first mistake in a page, so a page can have more after the one shown.'); +} + function reportMissingImages(images: MissingImage[], sourceDirectory: DirectoryPath): void { const lines = images.map(({ page, line, url, missing }) => { const where = ` • ${relative(page, sourceDirectory)}, line ${line}: ${f.var(url)}`; diff --git a/src/types/portal/content-tree.ts b/src/types/portal/content-tree.ts index 655c5d950..be26201cd 100644 --- a/src/types/portal/content-tree.ts +++ b/src/types/portal/content-tree.ts @@ -20,6 +20,7 @@ import { } from './portal-navigation.js'; import { ContentProblem, + InvalidPage, MissingImage, PortalSpec, ReservedAddress, @@ -170,6 +171,11 @@ export class ContentTree { if (read.missingImages.length > 0) { problems.push({ kind: 'missingImages', images: read.missingImages }); } + // Refused here too, so a preview hears of them, and a build of every page at once rather than the first. + const invalidPages = ContentTree.invalidPages(read.pages); + if (invalidPages.length > 0) { + problems.push({ kind: 'invalidMdx', pages: invalidPages }); + } // The walk takes each page to be served where it sits, so it would misjudge an entry naming one of these. if (addressProblems.length > 0) { @@ -227,6 +233,10 @@ export class ContentTree { }; } + private static invalidPages(pages: ReadPage[]): InvalidPage[] { + return pages.flatMap(({ file, parsed }) => (parsed?.syntaxError ? [{ page: file, ...parsed.syntaxError }] : [])); + } + /** * Every `nav.json` in the tree, validated against the directory it orders, plus any `nav.json` * in a case the build does not match. One walk, because both come from the same tree, and a diff --git a/src/types/portal/page.ts b/src/types/portal/page.ts index 7e423b4b3..d9282bbbc 100644 --- a/src/types/portal/page.ts +++ b/src/types/portal/page.ts @@ -22,10 +22,20 @@ export interface PageImage { path: string; } -/** A page as the build parses it: its front matter held to the build's schema, and the images the build imports. */ +/** Where an `.mdx` page stops being valid MDX, which fails the build, or leaves the page an error screen in a preview. */ +export interface PageSyntaxError { + line: number; + column: number; + /** As MDX gives it, less the parser's own names. */ + reason: string; +} + +/** A page as the build parses it: its front matter held to the build's schema, the images the build imports, and where it stops being valid MDX. */ export interface ParsedPage { frontMatter: Result; images: PageImage[]; + /** The first mistake in its body, if any: MDX reads no further. */ + syntaxError: PageSyntaxError | null; } type SchemaIssue = NonNullable['error']>['issues'][number]; @@ -40,23 +50,40 @@ interface MarkdownNode { children?: MarkdownNode[]; } +/** What MDX throws, a vfile message, as far as it says where and why. */ +interface ParseFailure { + reason?: unknown; + line?: unknown; + column?: unknown; + cause?: unknown; +} + // A scheme, a drive letter or a network path: none of them names a file the build imports. const NOT_A_FILE = /^(?:[a-z][a-z\d+.-]*:|\/\/)/i; -/** Parsed once, as the build parses it, `.mdx` as MDX; a body that does not parse is the build's to report. */ +// A tag's place, as MDX writes it into a reason: `(3:1-3:8)`. +const PLACE_IN_REASON = /\s*\((\d+):(\d+)-\d+:\d+\)$/; + +/** Parsed once, as the build parses it, `.mdx` as MDX. */ export async function parsePage(contents: string, fileName: FileName, label: string): Promise { let parsed: { matter: string; data: unknown }; try { parsed = frontmatter(contents); } catch (error) { const reason = errorMessage(error).split('\n')[0]; - return { frontMatter: err([`${label}: its front matter is not valid YAML. ${reason}`]), images: [] }; + return { + frontMatter: err([`${label}: its front matter is not valid YAML. ${reason}`]), + images: [], + syntaxError: null + }; } // Blanked rather than cut, so every line keeps its number. const body = '\n'.repeat(parsed.matter.split('\n').length - 1) + contents.slice(parsed.matter.length); + const tree = await parsedBody(body, fileName.hasExactExtension('.mdx')); return { frontMatter: await checkedFrontMatter(parsed, label), - images: await imagesIn(body, fileName.hasExactExtension('.mdx')) + images: tree.map(imagesIn).unwrapOr([]), + syntaxError: tree.isErr() ? tree.error : null }; } @@ -104,17 +131,38 @@ function titleError(title: unknown, label: string): string { return `${label}: 'title' must be text. Put it in quotes if it looks like a number or true or false.`; } -async function imagesIn(body: string, isMdx: boolean): Promise { +async function parsedBody(body: string, isMdx: boolean): Promise> { // On first use, as the schema is: most commands never read a page. const { remark } = await import('remark'); const processor = isMdx ? remark().use((await import('remark-mdx')).default) : remark(); - let tree: MarkdownNode; try { - tree = processor.parse(body) as MarkdownNode; - } catch { - return []; + return ok(processor.parse(body) as MarkdownNode); + } catch (error) { + return err(syntaxError(isJsonObject(error) ? error : {})); + } +} + +/** + * Where MDX stopped, and why. An unclosed tag is only found at the end of the page, so MDX names the + * tag's own place in its reason, and gives the error none. + */ +function syntaxError({ reason, line, column, cause }: ParseFailure): PageSyntaxError { + const text = typeof reason === 'string' ? reason : 'It could not be read as MDX.'; + const tag = PLACE_IN_REASON.exec(text); + const said = text.replace(PLACE_IN_REASON, '').replace(/ with acorn$/, ''); + // "Could not parse expression" says nothing of what was wrong with it; acorn's own reason does. + const detail = cause instanceof Error ? `: ${cause.message}` : ''; + if (typeof line === 'number' && typeof column === 'number') { + return { line, column, reason: said + (tag === null ? '' : ` on line ${tag[1]}`) + detail }; } + return { + line: tag === null ? 1 : Number(tag[1]), + column: tag === null ? 1 : Number(tag[2]), + reason: said + detail + }; +} +function imagesIn(tree: MarkdownNode): PageImage[] { // The template writes `![a][ref]` as `![a](url)` before the build imports it (remark-image-references.ts). const definitions = new Map(); const define = (node: MarkdownNode) => { diff --git a/src/types/portal/portal-source.ts b/src/types/portal/portal-source.ts index 064b524f8..1f30127ef 100644 --- a/src/types/portal/portal-source.ts +++ b/src/types/portal/portal-source.ts @@ -6,6 +6,7 @@ import { SuggestedSite } from './config/site-config.js'; import { ContentNotices } from './content-notices.js'; import { GeneratedPages } from './generated-pages.js'; import { ParseFailure } from './openapi-document.js'; +import { PageSyntaxError } from './page.js'; import { PortalConfig } from './portal-config.js'; import { Shared } from './shared.js'; import { UnresolvableReference } from './spec-bundle.js'; @@ -101,6 +102,11 @@ export interface MissingImage { missing: MissingFile | null; } +/** A page that is not valid MDX, at its first mistake. */ +export interface InvalidPage extends PageSyntaxError { + page: FilePath; +} + /** Why `content/` cannot be built; each variant maps to its own message. */ export type ContentProblem = | { kind: 'unreadableContent' } @@ -109,7 +115,8 @@ export type ContentProblem = | { kind: 'sharedAddresses'; addresses: SharedAddress[] } | { kind: 'invalidFrontMatter'; errors: string[] } | { kind: 'invalidNavigation'; errors: string[] } - | { kind: 'missingImages'; images: MissingImage[] }; + | { kind: 'missingImages'; images: MissingImage[] } + | { kind: 'invalidMdx'; pages: InvalidPage[] }; /** How `api transform` would turn the first document in a directory into one the portal reads. */ export interface SpecConversion { diff --git a/test/prompts/portal/source.test.ts b/test/prompts/portal/source.test.ts index 62fe5069f..4e351e65a 100644 --- a/test/prompts/portal/source.test.ts +++ b/test/prompts/portal/source.test.ts @@ -154,6 +154,41 @@ describe('reportSourceProblem', () => { }); }); + describe('pages that are not valid MDX', () => { + const cards = new FilePath(source.join('content').join('components'), new FileName('cards.mdx')); + const steps = new FilePath(source.join('content').join('components'), new FileName('steps.mdx')); + + it('names each by its page, line and column, and says there can be more in a page', () => { + reportContent({ + kind: 'invalidMdx', + pages: [ + { page: cards, line: 55, column: 1, reason: 'Expected a closing tag for ``' }, + { page: steps, line: 9, column: 12, reason: 'Could not parse expression: Unexpected token' } + ] + }); + + expect(printed()).to.contain('Pages in'); + expect(printed()).to.contain('are not valid MDX'); + expect(printed()).to.contain( + "'content/components/cards.mdx', line 55, column 1: Expected a closing tag for ``\n" + ); + expect(printed()).to.contain( + "'content/components/steps.mdx', line 9, column 12: Could not parse expression: Unexpected token" + ); + expect(printed()).to.contain('MDX stops at the first mistake in a page'); + }); + + it('speaks of one page when there is one', () => { + reportContent({ + kind: 'invalidMdx', + pages: [{ page: cards, line: 55, column: 1, reason: 'Expected a closing tag for ``' }] + }); + + expect(printed()).to.contain('A page in'); + expect(printed()).to.contain('is not valid MDX'); + }); + }); + describe('a spec directory the portal reads no document from', () => { const spec = source.join('spec'); const conversion = (format: string | null, others = 0) => ({ diff --git a/test/types/portal-source-context.test.ts b/test/types/portal-source-context.test.ts index 7b55d3e59..feeea61f8 100644 --- a/test/types/portal-source-context.test.ts +++ b/test/types/portal-source-context.test.ts @@ -2033,6 +2033,47 @@ describe('PortalSourceContext', () => { }); }); + // `portal generate` would fail over the first only, naming its temporary copy; `portal serve`, over none. + describe('pages that are not valid MDX', () => { + const cards = () => new FilePath(new DirectoryPath(root, 'content', 'components'), new FileName('cards.mdx')); + const OPEN_CARDS = page('Cards') + '\n\n \n'; + + beforeEach(() => { + writeConfig({ site: { name: 'Calc' } }); + write('spec/api.json', OPENAPI); + write('content/index.md', page('Home')); + }); + + it('refuses each one by its page, line and column', async () => { + write('content/components/cards.mdx', OPEN_CARDS); + write('content/components/steps.mdx', page('Steps') + '\nThen {oops(}.\n'); + + const problem = contentProblem((await resolve())._unsafeUnwrapErr()); + + expect(problem.kind === 'invalidMdx' && problem.pages).to.deep.equal([ + { page: cards(), line: 5, column: 1, reason: 'Expected a closing tag for ``' }, + { + page: new FilePath(new DirectoryPath(root, 'content', 'components'), new FileName('steps.mdx')), + line: 5, + column: 12, + reason: 'Could not parse expression: Unexpected token' + } + ]); + }); + + it('refuses one saved while the preview runs, and accepts it once fixed', async () => { + const { specs, generatedPages } = (await resolve())._unsafeUnwrap(); + const context = new PortalSourceContext(new DirectoryPath(root)); + + write('content/components/cards.mdx', OPEN_CARDS); + const refused = (await context.resolveContent(specs, generatedPages))._unsafeUnwrapErr(); + write('content/components/cards.mdx', OPEN_CARDS + '\n'); + + expect(refused.map(({ kind }) => kind)).to.deep.equal(['invalidMdx']); + expect((await context.resolveContent(specs, generatedPages)).isOk()).to.be.true; + }); + }); + describe('nav.json', () => { beforeEach(() => { writeConfig({ site: { name: 'Calc' } }); diff --git a/test/types/portal/content-tree.test.ts b/test/types/portal/content-tree.test.ts index f25ecc67e..2a742cbec 100644 --- a/test/types/portal/content-tree.test.ts +++ b/test/types/portal/content-tree.test.ts @@ -99,6 +99,35 @@ describe('ContentTree', () => { expect(problems[0]).to.deep.equal({ kind: 'missingImages', images: [image] }); }); + it('refuses every page that is not valid MDX at once, and none of those that are', async () => { + const cards = inContent('guides', 'cards.mdx'); + const steps = inContent('guides', 'steps.mdx'); + const pages = await parsed( + [inContent('index.md'), '---\ntitle: Home\n---\n'], + [cards, '---\ntitle: Cards\n---\n\n\n \n'], + [steps, '---\ntitle: Steps\n---\n\n\n\n'], + [inContent('guides', 'tabs.mdx'), '---\ntitle: Tabs\n---\n\n\n One\n\n'] + ); + const navigationFiles = handed([inContent('nav.json'), '{}'], [inContent('guides', 'nav.json'), '{}']); + + const problems = tree.check({ pages, navigationFiles, missingImages: [] }, [], GENERATED)._unsafeUnwrapErr(); + + expect(problems).to.deep.equal([ + { + kind: 'invalidMdx', + pages: [ + { page: cards, line: 5, column: 1, reason: 'Expected a closing tag for ``' }, + { + page: steps, + line: 6, + column: 1, + reason: 'Unexpected closing tag ``, expected corresponding closing tag for `` on line 5' + } + ] + } + ]); + }); + it('names the tabs from what it is handed', () => { const navigationFiles = handed( [inContent('nav.json'), JSON.stringify({ title: 'Guides', tabs: ['guides'], pages: ['index'] })], diff --git a/test/types/portal/page.test.ts b/test/types/portal/page.test.ts index a07ef80d7..ab7f43e3d 100644 --- a/test/types/portal/page.test.ts +++ b/test/types/portal/page.test.ts @@ -113,7 +113,7 @@ describe('parsePage', () => { expect((await images('---\ntitle: Home\n---\n\n![a](/a.png)\n')).map(({ line }) => line)).to.deep.equal([5]); }); - // The front matter check reports the first; the build reports the second. + // The front matter check reports the first; the syntax error, the second. it('finds nothing in a page that does not parse, rather than throwing', async () => { expect(await images('---\ntitle: [unclosed\n---\n![a](/a.png)\n')).to.deep.equal([]); expect(await images('\n\n![a](/a.png)\n', 'page.mdx')).to.deep.equal([]); @@ -149,4 +149,50 @@ describe('parsePage', () => { expect(found.map(({ path, line }) => [path, line])).to.deep.equal([['a.png', 3]]); }); }); + + // A preview leaves such a page an error screen, and the build fails over it naming its temporary copy. + describe('the first mistake in an .mdx page', () => { + const syntaxError = async (body: string, name = 'page.mdx') => + (await page(`---\ntitle: Cards\n---\n\n${body}`, name)).syntaxError; + + it('places a tag left open at the tag, which MDX finds only at the end of the page', async () => { + expect(await syntaxError('## One\n\n\n \n\n## Two\n')).to.deep.equal({ + line: 7, + column: 1, + reason: 'Expected a closing tag for ``' + }); + }); + + it('places a closing tag that does not match at the closing tag, naming the line of the one it should close', async () => { + expect(await syntaxError('\n Text\n\n')).to.deep.equal({ + line: 7, + column: 1, + reason: 'Unexpected closing tag ``, expected corresponding closing tag for `` on line 5' + }); + }); + + it("gives what is wrong with an expression in acorn's words, and not its name", async () => { + expect(await syntaxError('Then {oops(}.\n')).to.deep.equal({ + line: 5, + column: 12, + reason: 'Could not parse expression: Unexpected token' + }); + }); + + it('names an HTML comment as MDX does, with what to write instead', async () => { + const found = await syntaxError('\n'); + + expect(found).to.include({ line: 5, column: 2 }); + expect(found?.reason).to.contain('use `{/* text */}`'); + }); + + for (const [description, body, name] of [ + ['a valid page', '\n \n\n\n{/* a comment */}\n', 'page.mdx'], + ['a .md page, which is not read as MDX', '\n\n\n\nIf a <5.\n', 'page.md'] + ]) { + it(`finds none in ${description}`, async () => { + expect(await syntaxError(body, name)).to.be.null; + }); + } + }); });