-
Notifications
You must be signed in to change notification settings - Fork 40
Docs Renderer: Add local preview server #2437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a local development server for the @grafana/plugin-docs-renderer package, enabling plugin developers to preview multi-page documentation locally with optional live reload functionality. The server uses Express to serve GitHub-flavored markdown content based on a manifest.json structure.
Changes:
- Added Express-based development server with file watching and live reload capabilities
- Implemented recursive page lookup supporting nested documentation structures
- Added comprehensive test suite using supertest to validate server functionality
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
packages/plugin-docs-renderer/src/server.ts |
New Express server implementation with route handlers, file watching, and HTML generation |
packages/plugin-docs-renderer/src/server.test.ts |
Test suite covering server routes, static assets, live reload, and error cases |
packages/plugin-docs-renderer/src/index.ts |
Exports server functions and types for public API |
packages/plugin-docs-renderer/src/bin/run.ts |
CLI completely rewritten to start development server instead of processing docs |
packages/plugin-docs-renderer/src/__fixtures__/test-docs/* |
Test fixtures including manifest, markdown files, and static assets |
packages/plugin-docs-renderer/package.json |
Added express, chokidar, supertest dependencies and dev script |
package-lock.json |
Lock file updates for new dependencies |
| @@ -0,0 +1,23 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am assuming this will be generated from the docs when we actually run the renderer?
In that case we should have the renderer generate the manifest on the fly instead of having it here hardcoded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely! To prevent this pr from becoming massive I'll handle that in a follow-up, ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to push code that scans the docs folder and generates a manifest based on the filesystem structure + markdown metadata after all. Proper validation will be added in a follow up pr.
| ` | ||
| : ''; | ||
|
|
||
| return ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a fan of having an html template here. there are some minimal template engines that we can use for express js and we are going to eventually have to generate more and more htlm that we won't want to write here in JS files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 1d583f9. Went with EJS only because I've used it before.
| return; | ||
| } | ||
|
|
||
| debug('Request for slug: %s', slug); |
Check warning
Code scanning / CodeQL
Log injection Medium
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 16 hours ago
To fix the problem, sanitize the user-controlled slug before logging it. For plain-text logging, the key step is to remove newline and carriage-return characters (and optionally other non-printable control characters) so an attacker cannot create extra log lines or otherwise break log structure. We should perform this sanitization only for the log message, leaving the actual routing logic unchanged so functionality is preserved.
The best minimal fix here is to introduce a sanitized variant of slug right before logging: e.g. const safeSlugForLog = slug.replace(/[\r\n]/g, ''); and then pass safeSlugForLog to the debug calls that log the slug. This avoids changing how slug is used to look up pages, but ensures log output does not contain embedded newlines. We do not need extra libraries: a simple String.prototype.replace with a small regex is sufficient. Concretely:
- In
packages/plugin-docs-renderer/src/server/server.ts, within the route handler (app.get('*', ...)), after computingslugand before thedebug('Request for slug: %s', slug);call, computesafeSlugForLogby stripping\rand\n. - Use
safeSlugForLoginstead ofslugin thatdebugcall, and in the laterdebug('Page not found for slug: %s', ...)call as well (since that also logs the slug). - No new imports, methods, or types are required.
-
Copy modified lines R125-R127 -
Copy modified line R132
| @@ -122,12 +122,14 @@ | ||
| return; | ||
| } | ||
|
|
||
| debug('Request for slug: %s', slug); | ||
| // Sanitize slug for logging to prevent log injection via newlines | ||
| const safeSlugForLog = slug.replace(/[\r\n]/g, ''); | ||
| debug('Request for slug: %s', safeSlugForLog); | ||
|
|
||
| // find the file for this slug | ||
| const fileName = findPageBySlug(slug, manifest.pages); | ||
| if (!fileName) { | ||
| debug('Page not found for slug: %s', slug); | ||
| debug('Page not found for slug: %s', safeSlugForLog); | ||
| res.status(404).send('Page not found'); | ||
| return; | ||
| } |
| // find the file for this slug | ||
| const fileName = findPageBySlug(slug, manifest.pages); | ||
| if (!fileName) { | ||
| debug('Page not found for slug: %s', slug); |
Check warning
Code scanning / CodeQL
Log injection Medium
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 16 hours ago
To fix this class of problem, any user-controlled value that is written to logs should be sanitized so that it cannot inject new log entries or otherwise break the log format. For plain-text logs, the main concern is removing newline (\n) and carriage return (\r) characters (and optionally other control characters). The safest and least intrusive fix here is to create a small helper that normalizes/sanitizes strings before they are logged and apply it to the slug (and any similar user-derived values) used in logging statements.
Concretely for this file, we can add a local utility function, for example sanitizeForLog, near the top of the file. This helper will take a string (or unknown) and return a string with \r and \n removed (you can also strip other control chars if desired, but we’ll keep the minimal change). Then, on line 130, instead of passing slug directly into debug, we pass sanitizeForLog(slug). This preserves all existing behavior except that line breaks in the slug will be removed before being logged. No external dependencies are required; we can implement the sanitizer with String.prototype.replace. We will keep the rest of the logic (including how slugs are used to look up pages) unchanged to avoid altering runtime behavior outside logging.
-
Copy modified lines R15-R20 -
Copy modified line R136
| @@ -12,6 +12,12 @@ | ||
|
|
||
| const debug = createDebug('plugin-docs-renderer:server'); | ||
|
|
||
| function sanitizeForLog(value: unknown): string { | ||
| const str = String(value); | ||
| // Remove newline and carriage return characters to prevent log injection | ||
| return str.replace(/[\r\n]+/g, ''); | ||
| } | ||
|
|
||
| export interface ServerOptions { | ||
| docsPath: string; | ||
| port: number; | ||
| @@ -127,7 +133,7 @@ | ||
| // find the file for this slug | ||
| const fileName = findPageBySlug(slug, manifest.pages); | ||
| if (!fileName) { | ||
| debug('Page not found for slug: %s', slug); | ||
| debug('Page not found for slug: %s', sanitizeForLog(slug)); | ||
| res.status(404).send('Page not found'); | ||
| return; | ||
| } |
…mentation manifest
… manifest generation
- Add express and ejs for server functionality - Add chokidar for file watching - Add github-slugger and globby for scanner - Add supertest for testing HTTP endpoints - Add all corresponding @types packages These dependencies were lost during the rebase conflict resolution.
e7fbf3d to
c5910db
Compare
| expect(children[1].title).toBe('Database'); | ||
| }); | ||
|
|
||
| it('should store nested files with relative paths', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have a test case for a markdown file with broken or malformed metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 added
|
|
||
| // read and parse the file | ||
| const fileContent = await readFile(absolutePath, 'utf-8'); | ||
| const parsed = matter(fileContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can matter throw an error if it fails to parse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep invoking matter on malformed yaml throws. captured in unit tests now.
|
|
||
| // validate frontmatter has required fields | ||
| const frontmatter = parsed.data as Partial<Frontmatter>; | ||
| if (!frontmatter.title || !frontmatter.description || frontmatter.sidebar_position === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we making sidebar_position mandatory? IMO it should be optional and we just order on whatever order the come from the file system if not defined.
also not 100% sure why description is a mandatory field. do we display it in the sidebar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM to make description optional and fallback on fs order
description it's not used right now in the preview server, but for SEO reasons I think we need a meta description for the actual website.
| // validate frontmatter has required fields | ||
| const frontmatter = parsed.data as Partial<Frontmatter>; | ||
| if (!frontmatter.title || !frontmatter.description || frontmatter.sidebar_position === undefined) { | ||
| console.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is almost silently failing. we should be more aggressive on the feedback that the markdowns file are being ignored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a future PR coming soon, all validation rules will be added. those will run before the first thing in the serve command so we'll provide clear user feedback then
academo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my main concerns:
- review what we should actually make mandatory in the markdown frontmatter
- better error handling, things like
scanDocsFolderare called without a try/catch that could show us a readable error of what's failing - add test cases for error states (e.g. corrupted markdown, no files, etc..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 20 out of 23 changed files in this pull request and generated 10 comments.
What this PR does / why we need it:
This PR introduces a local development server for the new
@grafana/plugin-docs-rendererpackage, enabling plugin developers to preview their multi-page documentation locally before publishing/submitting to the catalog. The server uses theplugin-docs-rendererlib to generate GH flavoured markdown and serves pages based on a manifest.json structure. The server has optional live reload functionality through file watching. Also adding a couple of simple api tests for the express server.Also replacing the manually edited
manifest.jsonfile with an auto-generated file. There's a new scanner file that uses globby to recursively find all markdown files inside the docs folder. It does some light validation, but much more validation will be added in the next PR.The following things will be addressed in upcoming PRs:
Example usage:
Which issue(s) this PR fixes:
Part of the rich plugin documentation initiative. This implements the local preview workflow described in the design doc, allowing plugin authors to iterate on documentation with immediate visual feedback.
Fixes https://github.com/grafana/grafana-community-team/issues/739
Special notes for your reviewer: