feat: revamp copy as markdown #2589
Conversation
WalkthroughAdds a remote function getPageMarkdown(routeId) that reads src/routes/{routeId}/+page.markdoc and returns its UTF‑8 contents or null. Replaces the prior local getMarkdownContent utility with the remote function and updates layout load functions to call it. Introduces a rewritten CopyAsMarkdown Svelte component that uses the page store, getPageMarkdown, a dropdown/menu UI, clipboard copying with a writable copied state and a 2000ms reset, plus new CSS. Enables experimental remoteFunctions in svelte.config.js. Moves the copy UI into article headers and removes it from the table of contents. Updates the IconType union with many changed string literals. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧬 Code graph analysis (2)src/routes/docs/+layout.server.ts (3)
src/routes/blog/+layout.server.ts (3)
🪛 ESLintsrc/lib/remote/markdown.remote.ts[error] 10-10: 'e' is defined but never used. (@typescript-eslint/no-unused-vars) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/lib/remote/markdown.remote.ts (1)
13-14: Consider logging errors for debugging.The catch block silently swallows all errors, making it difficult to debug issues like permission errors, file system problems, or unexpected failures. Consider logging errors (excluding expected ENOENT cases) for observability.
Apply this diff to add error logging:
- } catch (e) { + } catch (e: any) { + // Log unexpected errors for debugging (exclude expected file-not-found cases) + if (e?.code !== 'ENOENT') { + console.error('Failed to read markdown file:', { routeId, error: e?.message }); + } return null; }Note: This also addresses the ESLint warning about the unused variable.
src/lib/components/blog/copy-as-markdown.svelte (2)
18-23: Consider validating markdown content before copying.The copy function copies
markdown.current ?? '', which means it will copy an empty string if the content is null. Consider adding a check to prevent copying empty content and provide user feedback.Apply this diff to add validation:
const copy = () => { + if (!markdown.current?.trim()) { + return; + } if (timeout) clearTimeout(timeout); - copyToClipboard(markdown.current ?? ''); + copyToClipboard(markdown.current); copied.set(true); timeout = setTimeout(() => copied.set(false), 2000); };
26-55: Consider adding error state handling.The component silently hides itself if markdown fails to load. Consider showing an error message or alternative UI to inform users when the markdown cannot be loaded, improving the user experience.
You could add error handling like this:
{#if markdown.loading} <!-- Optional: Show loading state --> {:else if markdown.error} <!-- Show error state --> <div class="text-caption text-secondary ml-4 p-1.5"> Unable to load markdown </div> {:else if markdown.current} <button ...> <!-- existing button content --> </button> {/if}Note: This assumes the remote function provides an
errorproperty. Verify the API shape of the query result.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/components/blog/copy-as-markdown.svelte(1 hunks)src/lib/remote/markdown.remote.ts(1 hunks)svelte.config.js(1 hunks)
🧰 Additional context used
🪛 ESLint
src/lib/remote/markdown.remote.ts
[error] 13-13: 'e' is defined but never used.
(@typescript-eslint/no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: assets
- GitHub Check: tests
🔇 Additional comments (1)
svelte.config.js (1)
67-69: LGTM! Remote functions support enabled.The addition of
remoteFunctions: trueis correctly placed in the experimental configuration and is required for the new markdown fetching functionality.
| let timeout: ReturnType<typeof setTimeout> | undefined = undefined; | ||
| const copy = () => { | ||
| if (timeout) clearTimeout(timeout); | ||
| copyToClipboard(markdown.current ?? ''); | ||
| copied.set(true); | ||
| timeout = setTimeout(() => copied.set(false), 2000); | ||
| }; |
There was a problem hiding this comment.
Resource leak: Timeout not cleaned up on component unmount.
The timeout is not cleared when the component unmounts, which could cause the callback to execute after the component is destroyed, potentially leading to memory leaks or errors.
Apply this diff to add cleanup:
const markdown = getPageMarkdown($page.route.id);
const copied = writable(false);
let timeout: ReturnType<typeof setTimeout> | undefined = undefined;
+
+ $effect(() => {
+ return () => {
+ if (timeout) clearTimeout(timeout);
+ };
+ });
const copy = () => {
if (timeout) clearTimeout(timeout);
copyToClipboard(markdown.current ?? '');
copied.set(true);
timeout = setTimeout(() => copied.set(false), 2000);
};🤖 Prompt for AI Agents
In src/lib/components/blog/copy-as-markdown.svelte around lines 16 to 23, the
component stores a setTimeout in `timeout` but never clears it on unmount; add
Svelte cleanup by importing onDestroy and in an onDestroy callback
clearTimeout(timeout) and set timeout = undefined so any pending timer is
cancelled when the component is destroyed. Ensure the import for onDestroy is
added if missing and the onDestroy handler safely checks `if (timeout)
clearTimeout(timeout)`.
src/lib/remote/markdown.remote.ts
Outdated
| export const getPageMarkdown = query( | ||
| 'unchecked', | ||
| async (routeId: string | null) => { | ||
| if (!routeId) return null; | ||
|
|
||
| try { | ||
| const basePath = join(process.cwd(), 'src', 'routes', routeId, '+page.markdoc'); | ||
| return await readFile(basePath, 'utf-8'); | ||
| } catch (e) { | ||
| return null; | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
Critical: Path traversal vulnerability.
The routeId parameter is directly interpolated into the file path without validation or sanitization. A malicious input containing .. segments (e.g., ../../etc/passwd) could allow reading arbitrary files from the filesystem.
Apply this diff to sanitize the route ID:
+import { normalize, relative } from 'path';
+
export const getPageMarkdown = query(
'unchecked',
async (routeId: string | null) => {
if (!routeId) return null;
try {
- const basePath = join(process.cwd(), 'src', 'routes', routeId, '+page.markdoc');
+ const routesDir = join(process.cwd(), 'src', 'routes');
+ const targetPath = normalize(join(routesDir, routeId, '+page.markdoc'));
+
+ // Ensure the resolved path is within the routes directory
+ const relativePath = relative(routesDir, targetPath);
+ if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) {
+ return null;
+ }
+
- return await readFile(basePath, 'utf-8');
+ return await readFile(targetPath, 'utf-8');
} catch (e) {
return null;
}
}
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getPageMarkdown = query( | |
| 'unchecked', | |
| async (routeId: string | null) => { | |
| if (!routeId) return null; | |
| try { | |
| const basePath = join(process.cwd(), 'src', 'routes', routeId, '+page.markdoc'); | |
| return await readFile(basePath, 'utf-8'); | |
| } catch (e) { | |
| return null; | |
| } | |
| } | |
| ); | |
| import { normalize, relative, isAbsolute } from 'path'; | |
| export const getPageMarkdown = query( | |
| 'unchecked', | |
| async (routeId: string | null) => { | |
| if (!routeId) return null; | |
| try { | |
| const routesDir = join(process.cwd(), 'src', 'routes'); | |
| const targetPath = normalize(join(routesDir, routeId, '+page.markdoc')); | |
| // Ensure the resolved path is within the routes directory | |
| const relativePath = relative(routesDir, targetPath); | |
| if (relativePath.startsWith('..') || isAbsolute(relativePath)) { | |
| return null; | |
| } | |
| return await readFile(targetPath, 'utf-8'); | |
| } catch { | |
| return null; | |
| } | |
| } | |
| ); |
🧰 Tools
🪛 ESLint
[error] 13-13: 'e' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
In src/lib/remote/markdown.remote.ts around lines 5 to 17, the routeId is used
directly in a filesystem path which allows path traversal; validate and sanitize
routeId before using it: reject or return null for null/empty values, disallow
segments like ".." or any path separators ("/" or "\"), enforce a strict
allowlist pattern (e.g., /^[a-z0-9-_]+$/i) or normalize and resolve the
candidate path and verify it starts with the expected base directory
(join(process.cwd(), 'src','routes')), and only then read the file; if
validation fails return null and log or handle the invalid routeId case.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/components/blog/copy-as-markdown.svelte (1)
21-92: Clear the copy timeout on unmount.
copystores a timeout but the component never clears it, so the callback can fire after destroy and leak. Please addonDestroy(or a rune cleanup) to cancel the pending timer.Apply this diff:
-import { writable } from 'svelte/store'; +import { onDestroy } from 'svelte'; +import { writable } from 'svelte/store'; @@ let timeout: ReturnType<typeof setTimeout> | undefined = undefined; const copy = () => { if (timeout) clearTimeout(timeout); copyToClipboard(markdown.current ?? ''); copied.set(true); timeout = setTimeout(() => copied.set(false), 2000); }; + +onDestroy(() => { + if (timeout) { + clearTimeout(timeout); + timeout = undefined; + } +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/icons/optimized/external-icon.svgis excluded by!**/*.svgsrc/icons/svg/external-icon.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
src/lib/components/blog/copy-as-markdown.svelte(1 hunks)src/lib/components/blog/table-of-contents.svelte(0 hunks)src/lib/components/ui/icon/types.ts(1 hunks)src/lib/layouts/DocsArticle.svelte(3 hunks)src/lib/layouts/DocsTutorial.svelte(2 hunks)src/lib/remote/markdown.remote.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/lib/components/blog/table-of-contents.svelte
🧰 Additional context used
🪛 ESLint
src/lib/remote/markdown.remote.ts
[error] 11-11: 'e' is defined but never used.
(@typescript-eslint/no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: tests
- GitHub Check: assets
src/lib/remote/markdown.remote.ts
Outdated
| export const getPageMarkdown = query('unchecked', async (routeId: string | null) => { | ||
| if (!routeId) return null; | ||
|
|
||
| try { | ||
| const basePath = join(process.cwd(), 'src', 'routes', routeId, '+page.markdoc'); | ||
| return await readFile(basePath, 'utf-8'); | ||
| } catch (e) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Fix path resolution and block traversal before shipping.
routeId arrives with leading /, so join(process.cwd(), 'src', 'routes', routeId, …) resolves to an absolute path outside your project (/docs/...). The read therefore fails for every normal route, and even worse, attackers can smuggle .. segments to walk the filesystem. Tighten the path handling (strip the leading slash, normalise, reject any attempt to leave src/routes) and drop the unused catch variable so lint passes.
Apply this diff:
-import { join } from 'path';
+import { join, normalize, relative, sep } from 'path';
export const getPageMarkdown = query('unchecked', async (routeId: string | null) => {
if (!routeId) return null;
try {
- const basePath = join(process.cwd(), 'src', 'routes', routeId, '+page.markdoc');
- return await readFile(basePath, 'utf-8');
- } catch (e) {
+ const routesDir = join(process.cwd(), 'src', 'routes');
+ const sanitizedRouteId = routeId.replace(/^\/+/, '');
+ const targetPath = normalize(join(routesDir, sanitizedRouteId, '+page.markdoc'));
+ const relativePath = relative(routesDir, targetPath);
+
+ if (
+ relativePath.startsWith('..') ||
+ relativePath.includes(`${sep}..`) ||
+ !targetPath.startsWith(routesDir)
+ ) {
+ return null;
+ }
+
+ return await readFile(targetPath, 'utf-8');
+ } catch {
return null;
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getPageMarkdown = query('unchecked', async (routeId: string | null) => { | |
| if (!routeId) return null; | |
| try { | |
| const basePath = join(process.cwd(), 'src', 'routes', routeId, '+page.markdoc'); | |
| return await readFile(basePath, 'utf-8'); | |
| } catch (e) { | |
| return null; | |
| } | |
| import { join, normalize, relative, sep } from 'path'; | |
| export const getPageMarkdown = query('unchecked', async (routeId: string | null) => { | |
| if (!routeId) return null; | |
| try { | |
| const routesDir = join(process.cwd(), 'src', 'routes'); | |
| const sanitizedRouteId = routeId.replace(/^\/+/, ''); | |
| const targetPath = normalize(join(routesDir, sanitizedRouteId, '+page.markdoc')); | |
| const relativePath = relative(routesDir, targetPath); | |
| if ( | |
| relativePath.startsWith('..') || | |
| relativePath.includes(`${sep}..`) || | |
| !targetPath.startsWith(routesDir) | |
| ) { | |
| return null; | |
| } | |
| return await readFile(targetPath, 'utf-8'); | |
| } catch { | |
| return null; | |
| } | |
| }); |
🧰 Tools
🪛 ESLint
[error] 11-11: 'e' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
In src/lib/remote/markdown.remote.ts around lines 5 to 13, routeId is passed
with a leading slash and current join(...) can produce an absolute path or allow
path traversal; strip any leading slashes from routeId, normalize and resolve
the candidate path against path.join(process.cwd(), 'src', 'routes') and then
verify the resolved path starts with that routes base directory (reject/return
null if it doesn't or if normalized routeId contains '..'); construct the final
file path only after this validation and use catch { } (no unused catch
variable) to return null on errors.
What does this PR do?
refactored the existing copy as markdown button
Test Plan
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
UI
Style