Conversation
|
Hello! 👋 This repository uses Auto for releasing packages using PR labels. ✨ This PR can be merged. It will not be considered when calculating future versions of the npm packages and will not appear in the changelogs. |
sunker
added
release
| 'no-redeclare': 'off', | ||
| // ts compiler handles redeclaration; using the same name for a const object and its derived type (e.g. `const Foo = {...} as const; type Foo = ...`) is valid ts but trips eslint | ||
| }, | ||
| }, |
There was a problem hiding this comment.
See
plugin-tools/packages/plugin-docs-cli/src/validation/types.ts
Lines 3 to 24 in 0340a63
sunker
requested review from
academo,
eledobleefe,
jackw,
leventebalogh and
mckn
There was a problem hiding this comment.
Pull request overview
This PR enhances the plugin docs toolchain by (1) enforcing frontmatter requirements via the docs CLI validator and (2) ensuring rendered pages don’t show a “double title” by stripping h1 headings from parsed markdown content.
Changes:
- Add a
rehype-strip-h1plugin to removeh1elements from parsed markdown and update parser/server tests accordingly. - Introduce frontmatter validation rules in
plugin-docs-cli(required fields, type checks, slug checks, duplicate slug, duplicate siblingsidebar_position, andh1warnings). - Standardize validator rule identifiers via a typed
Ruleconstant and update existing filesystem validation/tests to use it.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/plugin-docs-parser/src/types.ts | Make sidebar_position optional in the shared frontmatter type. |
| packages/plugin-docs-parser/src/plugins/rehype-strip-h1.ts | New rehype plugin that removes h1 nodes from the HAST tree. |
| packages/plugin-docs-parser/src/plugins/rehype-strip-h1.test.ts | Unit tests for the rehype-strip-h1 plugin behavior. |
| packages/plugin-docs-parser/src/parser.ts | Insert rehypeStripH1 into the markdown parsing pipeline. |
| packages/plugin-docs-parser/src/parser.test.ts | Update parser tests to reflect h1 stripping and add a targeted test. |
| packages/plugin-docs-cli/src/validation/types.ts | Add typed Rule identifiers and type diagnostics to use them. |
| packages/plugin-docs-cli/src/validation/rules/index.ts | Register the new frontmatter rule runner alongside filesystem rules. |
| packages/plugin-docs-cli/src/validation/rules/frontmatter.ts | New frontmatter validation rule implementation (field checks + duplicate checks). |
| packages/plugin-docs-cli/src/validation/rules/frontmatter.test.ts | Add test coverage for frontmatter rule diagnostics and duplicate detection. |
| packages/plugin-docs-cli/src/validation/rules/filesystem.ts | Switch filesystem diagnostics to use typed Rule values. |
| packages/plugin-docs-cli/src/validation/rules/filesystem.test.ts | Update filesystem tests to use Rule and add symlink / allowed-file-type tests. |
| packages/plugin-docs-cli/src/validation/format.test.ts | Update formatting tests to use typed Rule values. |
| packages/plugin-docs-cli/src/validation/engine.test.ts | Update engine tests to use typed Rule values. |
| packages/plugin-docs-cli/src/server/views/docs-layout.ejs | Render the page title in the layout and avoid duplicative h1 usage in the header. |
| packages/plugin-docs-cli/src/server/server.test.ts | Update server HTML assertions to match the new layout/page title behavior. |
| packages/plugin-docs-cli/src/scanner.ts | Stop requiring sidebar_position as a mandatory frontmatter field during scanning. |
| eslint.config.js | Disable no-redeclare for plugin-docs-cli TS/TSX files to support const Foo + type Foo patterns. |
| package-lock.json | Lockfile update reflecting dependency metadata changes. |
grafana-plugins-platform-bot
bot
moved this from 📬 Triage
to 🔬 In review
in Grafana Catalog Team
| const findings = await checkFrontmatter(input(invalidDocs)); | ||
|
|
||
| const typeErrors = findings.filter((f) => f.rule === Rule.FieldTypes); | ||
| expect(typeErrors.length).toBeGreaterThanOrEqual(3); |
There was a problem hiding this comment.
Use toBeEqual(3) because if something changes in the code that causes side effects this test will not catch it due to the way you are doing validations of contents in the next lines.
| // title on line 2, description on line 3 | ||
| await writeFile(join(tmp, 'page.md'), '---\ntitle: 123\ndescription: Valid\nsidebar_position: 1\n---\n'); | ||
|
|
||
| const findings = await checkFrontmatter(input(tmp)); |
There was a problem hiding this comment.
I am writing it only in this test but it applies for al the other tests: check how many findings there are and make it strict to that number of findings.
The problem is you are using .find in an array to check if there's an specific issue but that means if findings has more values (which is unexpected for this test) it won't fail
What this PR does / why we need it:
This PR adds frontmatter validation to the docs CLI and a
rehype-strip-h1plugin to the docs parser. The validator checks that every markdown page has a frontmatter block withtitleanddescriptionas strings, catches wrong types, flags invalid or duplicate custom slugs and detects duplicatesidebar_positionvalues among sibling pages. The h1-stripping plugin ensures rendered pages don't show a double title — since the page title comes from frontmatter any# Headingin the body is silently dropped by the parser.sidebar_positionis optional - pages without it sort last.Which issue(s) this PR fixes:
Part of https://github.com/grafana/grafana-catalog-team/issues/769
Special notes for your reviewer:
no-redeclareESLint override is scoped toplugin-docs-clionly, to allow theconst Foo = {} as const; type Foo = ...pattern intypes.ts