-
Notifications
You must be signed in to change notification settings - Fork 299
fix: added a registry pattern mechanism for dyamic markdown generation #2717
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
fix: added a registry pattern mechanism for dyamic markdown generation #2717
Conversation
WalkthroughAdds a dynamic markdown generation system and integrates it into existing markdown resolution. Introduces a route-pattern registry and utilities to register and invoke markdown generators. Adds two generators that produce markdown for API models and services from OpenAPI-like data. Updates getMarkdownContent to prefer dynamic generators before falling back to static +page.markdoc files. Modifies a copy-as-markdown component to derive the route identifier from the page URL pathname. No exported signatures were removed; two new generator functions are exported. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes RationaleThe diff adds multiple new modules and wiring but follows a consistent pattern: a registry, two generator implementations, and a single integration point in markdown resolution plus a small component tweak. The generators contain nontrivial logic for schema traversal and document assembly, requiring focused review of correctness and edge cases, but changes are localized and homogeneous in style, keeping overall review complexity moderate. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/lib/server/markdown-generators.ts`:
- Around line 19-31: The regex patterns registered via markdownGenerators.set
use unnecessary escapes inside character classes (e.g., [^\/]+) which triggers
the no-useless-escape lint rule; update both registrations that call
generateModelMarkdown and generateServiceMarkdown to remove the backslash inside
the character classes (change [^\/]+ to [^/]+) while leaving the rest of each
pattern and the corresponding function calls (generateModelMarkdown,
generateServiceMarkdown) unchanged.
In `@src/lib/server/markdown.ts`:
- Around line 14-15: The call site in copy-as-markdown.svelte should stop
falling back to page.route.id because that contains route patterns (e.g.,
/docs/products/[slug]) and breaks matching in
getPageMarkdown/getMarkdownContent; update the call to pass page.url.pathname
only (remove the "|| page.route.id" fallback) so getPageMarkdown and
getMarkdownContent receive a concrete pathname string; check that
getPageMarkdown/getMarkdownContent signatures accept a non-null string and
adjust types if necessary (refer to getPageMarkdown, getMarkdownContent,
page.url.pathname, and page.route.id).
🧹 Nitpick comments (2)
src/lib/server/markdown.ts (1)
17-22: Prefer a single-pass dynamic lookup with fallback on null.This avoids two regex scans and lets static markdoc act as a fallback if the generator returns null.
♻️ Proposed change
- if (hasDynamicMarkdownGenerator(routeId)) { - return await generateDynamicMarkdown(routeId); - } + const dynamic = await generateDynamicMarkdown(routeId); + if (dynamic !== null) return dynamic;src/lib/server/model-markdown.ts (1)
74-129: Guard against non-$refentries inanyOf/oneOf.The code assumes all
anyOf/oneOfitems contain a$refproperty, but OpenAPI allows inline schemas. This will cause a runtime error if the external OpenAPI specs contain inline schemas. Filter to only$refentries before calling.split().♻️ Suggested guard
- const refs = (property.items as OpenAPIV3.SchemaObject).anyOf?.map((item) => - ((item as OpenAPIV3.ReferenceObject).$ref as string).split('/').pop() - ); + const refs = (property.items as OpenAPIV3.SchemaObject).anyOf + ?.map((item) => ('$ref' in item ? item.$ref.split('/').pop() : undefined)) + .filter(Boolean) as string[] | undefined;Apply the same pattern to the
oneOfhandler at lines 111-114.
| export const getMarkdownContent = async (routeId: string | null) => { | ||
| if (!routeId) return null; |
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.
🧩 Analysis chain
🏁 Script executed:
rg -n "getMarkdownContent" -C3Repository: appwrite/website
Length of output: 1862
🏁 Script executed:
# Search for page.route.id usage
rg "page\.route\.id" -C2
# Also look at the markdown.ts implementation
cat -n src/lib/server/markdown.ts | head -50Repository: appwrite/website
Length of output: 6499
Remove fallback to page.route.id in copy-as-markdown.svelte.
The getPageMarkdown call uses page.url.pathname || page.route.id, but page.route.id contains the route pattern (e.g., /docs/products/[slug]), not a concrete pathname. This would cause the function to fail when trying to match dynamic markdown generators or load the corresponding file. Always pass page.url.pathname directly since it is always available.
🤖 Prompt for AI Agents
In `@src/lib/server/markdown.ts` around lines 14 - 15, The call site in
copy-as-markdown.svelte should stop falling back to page.route.id because that
contains route patterns (e.g., /docs/products/[slug]) and breaks matching in
getPageMarkdown/getMarkdownContent; update the call to pass page.url.pathname
only (remove the "|| page.route.id" fallback) so getPageMarkdown and
getMarkdownContent receive a concrete pathname string; check that
getPageMarkdown/getMarkdownContent signatures accept a non-null string and
adjust types if necessary (refer to getPageMarkdown, getMarkdownContent,
page.url.pathname, and page.route.id).
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.
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| export const getMarkdownContent = async (routeId: string | null) => { | ||
| if (!routeId) return null; |
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.
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.