Added logic for alias recognition to VitePress#925
Open
n-boshnakov wants to merge 4 commits intogardener:masterfrom
Open
Added logic for alias recognition to VitePress#925n-boshnakov wants to merge 4 commits intogardener:masterfrom
n-boshnakov wants to merge 4 commits intogardener:masterfrom
Conversation
|
@n-boshnakov: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for gardener-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
gardener-prow
bot
added
do-not-merge/needs-kind
Contributor
📝 WalkthroughWalkthroughAdds an alias-redirect feature: parses Markdown frontmatter Changes
Sequence Diagram(s)sequenceDiagram
participant MF as Markdown Files
participant Build as VitePress Build
participant AP as Alias Plugin
participant Dev as Dev Server
participant FS as Output Filesystem
rect rgba(100,150,200,0.5)
Note over MF,AP: Parse and map aliases from source
Build->>MF: Scan `srcDir` for .md
Build->>AP: Request alias extraction
AP->>AP: Read frontmatter, normalize & dedupe aliases
end
rect rgba(150,200,100,0.5)
Note over Dev,AP: Dev-server redirect handling
Dev->>AP: Incoming GET/HEAD (basePath stripped)
AP->>AP: Lookup alias map
AP-->>Dev: Respond 302 with Location header (if match)
end
rect rgba(200,150,100,0.5)
Note over Build,FS: Static redirect generation
Build->>AP: buildEnd(siteConfig) triggers generation
AP->>FS: Write HTML redirect files under outDir
FS-->>Build: Redirect files included in site output
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.vitepress/config.mts (1)
222-299: Consider adding type annotations for Vite server APIs to improve IDE support.The dev redirect plugin logic is sound with good caching, proper cache invalidation on file changes, and correct middleware ordering (calls
next()when no redirect is found, allowing other handlers to process the request). Error handling gracefully falls through to the next middleware.The
anytypes forserver,req, andresreduce type safety. Vite and Node.js provide standard type exports that should be used:🛠️ Optional: Add type imports for better IDE support
+import type { ViteDevServer } from 'vite' +import type { IncomingMessage, ServerResponse } from 'node:http' function createAliasRedirectDevPlugin(srcDir: string, basePath: string) { // ... return { name: 'vitepress-aliases-dev-redirects', apply: 'serve', - configureServer(server: any) { + configureServer(server: ViteDevServer) { // ... - server.middlewares.use((req: any, res: any, next: () => void) => { + server.middlewares.use((req: IncomingMessage, res: ServerResponse, next: () => void) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vitepress/config.mts around lines 222 - 299, The plugin uses untyped any for Vite server and HTTP req/res; add proper type annotations to improve IDE support: import ViteDevServer from 'vite' and use it for the configureServer(server) parameter in createAliasRedirectDevPlugin, and annotate the middleware handler parameters (req, res, next) with appropriate Node types (http.IncomingMessage / http.ServerResponse) or Connect types so IDEs can resolve properties like url and method; update signatures for configureServer(server: ViteDevServer) and the middleware callback inside configureServer (and refreshOnMarkdownChange/loadAliases references remain the same) and adjust any watcher/middleware usages to match the typed APIs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vitepress/config.mts:
- Around line 28-45: createRedirectHtml currently embeds targetPath via
JSON.stringify directly into a <script>, which allows breaking out if targetPath
contains "</script>" (XSS). Fix by creating a script-safe string (e.g., let
safeScript = JSON.stringify(targetPath).replace(/<\/script>/gi,
'<\\/script>').replace(/\u2028/g, '\\u2028').replace(/\u2029/g, '\\u2029')) and
use safeScript inside the window.location.replace(...) call while keeping the
HTML-escaped escapedTarget for the meta/link/body; update createRedirectHtml to
use this safeScript instead of JSON.stringify(targetPath).
---
Nitpick comments:
In @.vitepress/config.mts:
- Around line 222-299: The plugin uses untyped any for Vite server and HTTP
req/res; add proper type annotations to improve IDE support: import
ViteDevServer from 'vite' and use it for the configureServer(server) parameter
in createAliasRedirectDevPlugin, and annotate the middleware handler parameters
(req, res, next) with appropriate Node types (http.IncomingMessage /
http.ServerResponse) or Connect types so IDEs can resolve properties like url
and method; update signatures for configureServer(server: ViteDevServer) and the
middleware callback inside configureServer (and
refreshOnMarkdownChange/loadAliases references remain the same) and adjust any
watcher/middleware usages to match the typed APIs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.vitepress/plugins/alias-redirects.ts (1)
11-21:⚠️ Potential issue | 🔴 CriticalMake the inline redirect script safe for
</script>sequences.
JSON.stringify()is not enough inside a<script>tag. If a page path or configured base ever contains</script>, the generated redirect page becomes executable markup.🔒 Proposed fix
const createRedirectHtml = (targetPath: string) => { const escapedTarget = escapeHtml(targetPath) + const safeScriptTarget = JSON.stringify(targetPath) + .replace(/</g, '\\u003c') + .replace(/\u2028/g, '\\u2028') + .replace(/\u2029/g, '\\u2029') return `<!doctype html> <html lang="en"> <head> <meta charset="utf-8"> <title>Redirecting...</title> <meta http-equiv="refresh" content="0; url=${escapedTarget}"> <link rel="canonical" href="${escapedTarget}"> - <script>window.location.replace(${JSON.stringify(targetPath)})</script> + <script>window.location.replace(${safeScriptTarget})</script> </head>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vitepress/plugins/alias-redirects.ts around lines 11 - 21, The inline redirect script in createRedirectHtml is unsafe because JSON.stringify(targetPath) alone can still allow a closing "</script>" sequence to break out of the script tag; update createRedirectHtml to sanitize the string used inside the script by escaping any "</script>" occurrences (e.g. replace "</script>" with "<\/script>") after JSON.stringify(targetPath) (and likewise ensure escapedTarget used in attributes is properly HTML-escaped), so the generated script and markup cannot be terminated by a malicious path value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vitepress/plugins/alias-redirects.ts:
- Around line 98-107: Reject aliases containing dot-segments before joining them
into outDir: in toRedirectOutputPath, after computing normalizedAliasPath check
for any ".." path segment (e.g., normalizedAliasPath === '/..' or
normalizedAliasPath.includes('/..') or by splitting on '/' and detecting '..')
and return null (or otherwise refuse to create a redirect) if found to prevent
path traversal; apply the same validation to the other path-joining code in this
file that builds redirect output paths (the second occurrence around the other
redirect helper) so no alias can escape siteConfig.outDir.
---
Duplicate comments:
In @.vitepress/plugins/alias-redirects.ts:
- Around line 11-21: The inline redirect script in createRedirectHtml is unsafe
because JSON.stringify(targetPath) alone can still allow a closing "</script>"
sequence to break out of the script tag; update createRedirectHtml to sanitize
the string used inside the script by escaping any "</script>" occurrences (e.g.
replace "</script>" with "<\/script>") after JSON.stringify(targetPath) (and
likewise ensure escapedTarget used in attributes is properly HTML-escaped), so
the generated script and markup cannot be terminated by a malicious path value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62231597-0a4f-477b-88b4-66e7b226caf4
📒 Files selected for processing (2)
.vitepress/config.mts.vitepress/plugins/alias-redirects.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.vitepress/plugins/alias-redirects.ts (1)
57-60:⚠️ Potential issue | 🟠 MajorReject
.segments too.
path.join()normalizes the joined path, including.segments, so aliases like/./or/docs/./faq/still resolve insideoutDirand can overwrite the root or a real page. The current helper only blocks... (nodejs.org)🛡️ Proposed fix
function hasDotSegment(urlPath: string): boolean { - if (urlPath === '/..' || urlPath.includes('/../')) return true - return urlPath.split('/').some((segment) => segment === '..') + return urlPath.split('/').some((segment) => segment === '.' || segment === '..') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vitepress/plugins/alias-redirects.ts around lines 57 - 60, The helper hasDotSegment currently only rejects '..' segments; update it to also reject single-dot segments so paths containing '/./', '/.' or '/docs/./faq' are considered invalid. Locate the hasDotSegment function and change its logic to check segments for segment === '.' || segment === '..' (e.g., split on '/' and test for either '.' or '..'), ensuring edge cases like a trailing '/.' or occurrences anywhere in the path are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.vitepress/plugins/alias-redirects.ts:
- Around line 57-60: The helper hasDotSegment currently only rejects '..'
segments; update it to also reject single-dot segments so paths containing
'/./', '/.' or '/docs/./faq' are considered invalid. Locate the hasDotSegment
function and change its logic to check segments for segment === '.' || segment
=== '..' (e.g., split on '/' and test for either '.' or '..'), ensuring edge
cases like a trailing '/.' or occurrences anywhere in the path are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a662704-9272-4c50-995b-1820e31a34f8
📒 Files selected for processing (1)
.vitepress/plugins/alias-redirects.ts
Member
|
@n-boshnakov maybe we could print a list of the aliases created :) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vitepress/plugins/alias-redirects.ts:
- Around line 265-271: The loadAliases function stores a promise in
cachedAliases that can stay rejected and block future loads; update loadAliases
so that when you call collectAliasEntries(srcDir, basePath) you attach a .catch
handler that clears cachedAliases (set to undefined) before rethrowing the
error, ensuring a subsequent call retries; reference the loadAliases function,
the cachedAliases variable and collectAliasEntries call when implementing this
change.
- Around line 135-153: collectMarkdownFiles relies on the nondeterministic order
of fs.readdir which makes alias conflict resolution flaky; to fix, make the
traversal deterministic by sorting the directory entries before processing
(e.g., sort the entries array by entry.name with a consistent comparator) so
that collectMarkdownFiles always returns the same ordering and results (or
alternatively sort the final results array before returning); update the code
around the fs.readdir call and the entries handling to use the stable sort so
duplicate alias winners are deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82c59f92-b9ea-4b77-8703-b73d3d941934
📒 Files selected for processing (2)
.vitepress/config.mts.vitepress/plugins/alias-redirects.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .vitepress/config.mts
Comment on lines
+135
to
+153
| async function collectMarkdownFiles(rootDir: string): Promise<string[]> { | ||
| const results: string[] = [] | ||
| const entries = await fs.readdir(rootDir, { withFileTypes: true }) | ||
|
|
||
| for (const entry of entries) { | ||
| const absolutePath = path.join(rootDir, entry.name) | ||
|
|
||
| if (entry.isDirectory()) { | ||
| results.push(...await collectMarkdownFiles(absolutePath)) | ||
| continue | ||
| } | ||
|
|
||
| if (entry.isFile() && entry.name.toLowerCase().endsWith('.md')) { | ||
| results.push(absolutePath) | ||
| } | ||
| } | ||
|
|
||
| return results | ||
| } |
Contributor
There was a problem hiding this comment.
Sort the markdown scan before resolving duplicate aliases.
Line 137 relies on fs.readdir() order, but that order is not guaranteed. Because Lines 194-197 keep the first conflicting mapping, the winning alias target can vary across platforms and builds.
Proposed fix
async function collectMarkdownFiles(rootDir: string): Promise<string[]> {
const results: string[] = []
- const entries = await fs.readdir(rootDir, { withFileTypes: true })
+ const entries = (await fs.readdir(rootDir, { withFileTypes: true }))
+ .sort((a, b) => a.name.localeCompare(b.name))
for (const entry of entries) {
const absolutePath = path.join(rootDir, entry.name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vitepress/plugins/alias-redirects.ts around lines 135 - 153,
collectMarkdownFiles relies on the nondeterministic order of fs.readdir which
makes alias conflict resolution flaky; to fix, make the traversal deterministic
by sorting the directory entries before processing (e.g., sort the entries array
by entry.name with a consistent comparator) so that collectMarkdownFiles always
returns the same ordering and results (or alternatively sort the final results
array before returning); update the code around the fs.readdir call and the
entries handling to use the stable sort so duplicate alias winners are
deterministic.
Comment on lines
+265
to
+271
| const loadAliases = () => { | ||
| if (!cachedAliases) { | ||
| cachedAliases = collectAliasEntries(srcDir, basePath) | ||
| } | ||
|
|
||
| return cachedAliases | ||
| } |
Contributor
There was a problem hiding this comment.
Clear failed alias loads from the dev cache.
If collectAliasEntries() rejects once, cachedAliases stays as a rejected promise and every later request reuses the same failure until a watcher event invalidates it. Resetting the cache on rejection lets the next request retry.
Proposed fix
const loadAliases = () => {
if (!cachedAliases) {
- cachedAliases = collectAliasEntries(srcDir, basePath)
+ cachedAliases = collectAliasEntries(srcDir, basePath).catch((error) => {
+ cachedAliases = null
+ throw error
+ })
}
return cachedAliases
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vitepress/plugins/alias-redirects.ts around lines 265 - 271, The
loadAliases function stores a promise in cachedAliases that can stay rejected
and block future loads; update loadAliases so that when you call
collectAliasEntries(srcDir, basePath) you attach a .catch handler that clears
cachedAliases (set to undefined) before rethrowing the error, ensuring a
subsequent call retries; reference the loadAliases function, the cachedAliases
variable and collectAliasEntries call when implementing this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
How to categorize this PR?
/kind documentation
What this PR does / why we need it:
This PR adds alias redirect infrastructure to the website.
Which issue(s) this PR fixes:
Fixes #926
Special notes for your reviewer:
Summary by CodeRabbit
New Features
Refactor