-
Notifications
You must be signed in to change notification settings - Fork 297
Move markdown handling to server hooks #2678
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
Conversation
WalkthroughThis pull request refactors markdown content serving by migrating logic from a dedicated route handler to a new middleware hook. A Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks.server.ts (1)
225-232: MovemarkdownHandlerafterwwwRedirecterto ensure consistent subdomain behavior.The current ordering allows markdown files to be served from the
wwwsubdomain before the redirect occurs. This creates inconsistent behavior wherewww.domain.com/docs/guide.mdserves content instead of redirecting todomain.com/docs/guide.mdfirst.🔎 Proposed fix
export const handle = sequence( Sentry.sentryHandle(), - markdownHandler, redirecter, wwwRedirecter, + markdownHandler, securityheaders, initSession );
🧹 Nitpick comments (2)
src/hooks.server.ts (2)
18-20: Consider removing redundant variable assignment.The intermediate
withoutExtvariable is assigned torouteIdbut could be used directly, orrouteIdcould be assigned the result ofreplacedirectly.🔎 Proposed refactor
// strip trailing ".md" from the pathname to get the underlying route id - const withoutExt = pathname.replace(/\.md$/, ''); - const routeId = withoutExt; + const routeId = pathname.replace(/\.md$/, '');
24-24: Consider settingContent-Typeheader for 404 response.For consistency, the 404 response could specify a
Content-Typeheader, similar to the success response.🔎 Proposed refactor
- return new Response('Not found', { status: 404 }); + return new Response('Not found', { + status: 404, + headers: { + 'Content-Type': 'text/plain; charset=utf-8' + } + });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
src/hooks.server.tssrc/routes/[...slug]/+server.ts
💤 Files with no reviewable changes (1)
- src/routes/[...slug]/+server.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-06T19:20:26.393Z
Learnt from: ebenezerdon
Repo: appwrite/website PR: 2573
File: src/lib/utils/blog-mid-cta.ts:49-60
Timestamp: 2025-11-06T19:20:26.393Z
Learning: In the appwrite/website repository's blog system, markdown headings use a custom level mapping where single `#` translates to H2 (section heading) rather than the standard H1. The regex pattern `^(?:\s{0,3})#\s+.+$/gm` is used to match these section headings correctly in `src/lib/utils/blog-mid-cta.ts`.
Applied to files:
src/hooks.server.ts
🔇 Additional comments (1)
src/hooks.server.ts (1)
5-5: VerifygetMarkdownContenthandles path traversal and errors.Ensure that
getMarkdownContentvalidates the route ID to prevent path traversal attacks (e.g.,../../sensitive-file.md) and handles errors gracefully. The function call at line 22 should also be wrapped in error handling.
Moving the markdown handling to server hooks, since server endpoints dont use the
+error.sveltecomponent.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.