Add inline library previews and PDF exports#5
Open
Acex187x wants to merge 2 commits into
Open
Conversation
- Render Markdown, Office, PDF, and text previews in the file detail view - Add thumbnail generation, PDF conversion endpoints, and MDX preview plumbing - Update file cards to show generated thumbnails and file-type icons
📝 WalkthroughWalkthroughAdds a complete file thumbnail and preview system to the library app. A new standalone ChangesLibrary Thumbnail & Preview Pipeline
Sequence Diagram(s)sequenceDiagram
participant Browser
participant NextApp as Next.js App
participant ConvexDB as Convex (files)
participant ThumbSvc as Thumbnailer Service
participant Gotenberg
participant Storage as Convex Storage
Browser->>NextApp: Upload file
NextApp->>ConvexDB: files.create (thumbnailStatus=pending)
ConvexDB->>ConvexDB: scheduler.runAfter(thumbnails.generate)
ConvexDB->>ThumbSvc: POST /render {url, contentType}
alt Office document
ThumbSvc->>Gotenberg: LibreOffice convert → PDF
Gotenberg-->>ThumbSvc: PDF bytes
else Markdown/MDX
ThumbSvc->>NextApp: GET /internal/md-thumb?src=&token=
NextApp-->>ThumbSvc: rendered HTML screenshot
end
ThumbSvc-->>ConvexDB: JPEG + PDF buffers
ConvexDB->>Storage: store thumbnails
ConvexDB->>ConvexDB: attachThumbnail (thumbnailStatus=completed)
Browser->>NextApp: GET /api/pdf?fileId=&style=mdx
NextApp->>ConvexDB: files.get + downloadUrl
ConvexDB-->>NextApp: file metadata
NextApp->>ThumbSvc: POST /pdf {url, contentType, style}
ThumbSvc-->>NextApp: PDF bytes
NextApp-->>Browser: application/pdf
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
packages/backend/convex/schema.ts-291-294 (1)
291-294:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the lifecycle comment for Office files.
thumbnailKind()and the generator both support Office thumbnails, so listingofficeundernonecontradicts the new pipeline and can mislead future schema/API changes.Proposed fix
- * none — file type isn't thumbnailable (office/video/text/…) + * none — file type isn't thumbnailable (video/audio/unsupported text/…)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/backend/convex/schema.ts` around lines 291 - 294, The comment describing the thumbnail lifecycle states that office files are not thumbnailable (listed under the `none` state), but this contradicts the current implementation where `thumbnailKind()` and the generator both support Office thumbnails. Remove "office" from the list of file types shown in the `none` lifecycle state comment to accurately reflect that Office files are now supported by the thumbnail pipeline.apps/library/app/file/[id]/page.tsx-346-346 (1)
346-346:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove
shadow-xlfrom the PDF preview tooltip.This new tooltip uses the only forbidden shadow utility outside the Search dialog exception. Keep it border-based (or a lighter allowed treatment) instead.
As per coding guidelines, "Search dialog: use fixed inset-0 z-50 with rgba(0,0,0,0.5) backdrop, max-w-2xl, border, and shadow-xl (only allowed shadow exception)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/library/app/file/`[id]/page.tsx at line 346, The span element that creates the PDF preview tooltip contains the shadow-xl utility class on line 346 of apps/library/app/file/[id]/page.tsx, which violates the coding guidelines that restrict shadow-xl usage to the Search dialog only. Remove the shadow-xl class from the className attribute of this tooltip span while keeping the other styling (border, border-line-soft, bg-surface, and other utilities) intact.Source: Coding guidelines
apps/library/components/file-type-icon.tsx-50-62 (1)
50-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace hardcoded icon colors with theme variables.
The
COLORSmap hardcodes multiple hex values, which breaks the project styling contract for TSX components. Move these to theme variables (or subject palette constants) and consume via@/lib/theme.As per coding guidelines, "Use CSS variables (C.bg, C.bgWhite, C.text, C.textMuted, C.accent, C.border, C.borderLight, C.headerBg, C.headerText) imported from
@/lib/themefor all styling; never hardcode colors except for subject palette colors."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/library/components/file-type-icon.tsx` around lines 50 - 62, The COLORS constant in file-type-icon.tsx contains hardcoded hex color values which violates the styling guidelines. Replace all hardcoded hex values in the COLORS map with CSS variables imported from `@/lib/theme` (such as C.bg, C.bgWhite, C.text, C.textMuted, C.accent, C.border, C.borderLight, C.headerBg, C.headerText, or other appropriate theme variables). First add the necessary import statement for the theme variables at the top of the file, then update each color entry in the COLORS object to reference the corresponding theme variable instead of its hardcoded hex equivalent.Source: Coding guidelines
apps/library/components/file-preview.tsx-11-11 (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwap hardcoded white/black color utilities for theme variables.
The new preview surfaces use hardcoded white/black color classes; these should be expressed via theme variables/tokens from
@/lib/themeto keep light/dark theming and visual consistency aligned with the design system.As per coding guidelines, "Use CSS variables (C.bg, C.bgWhite, C.text, C.textMuted, C.accent, C.border, C.borderLight, C.headerBg, C.headerText) imported from
@/lib/themefor all styling; never hardcode colors except for subject palette colors."Also applies to: 60-60, 73-73, 113-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/library/components/file-preview.tsx` at line 11, Replace hardcoded color utility classes with theme color variables from `@/lib/theme` throughout the file. In the FRAME constant definition, replace the hardcoded bg-white class with the appropriate theme variable such as C.bgWhite. Apply the same approach to identify and replace any other hardcoded white or black color utilities elsewhere in the file with their corresponding theme color tokens (e.g., C.bg, C.text, C.textMuted, C.border, C.borderLight, etc.) to ensure consistency with the design system and proper light/dark theme support.Source: Coding guidelines
apps/library/lib/preview.ts-75-77 (1)
75-77:⚠️ Potential issue | 🟡 MinorUse own-property lookup for extension mapping.
ext in KIND_BY_EXTmatches prototype-chain keys. With a crafted filename (e.g.,file.__proto__), the extracted extension could be__proto__, bypassing the intended map and returning non-PreviewKindvalues at runtime. Use an own-property check before indexing.Suggested fix
- if (ext && ext in KIND_BY_EXT) { + if (ext && Object.prototype.hasOwnProperty.call(KIND_BY_EXT, ext)) { if (ext === "txt" && ct.startsWith("text/")) return "text"; return KIND_BY_EXT[ext]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/library/lib/preview.ts` around lines 75 - 77, The condition using the `in` operator for extension lookup in the KIND_BY_EXT mapping checks the entire prototype chain, not just own properties, which creates a security vulnerability where a crafted filename like `file.__proto__` could return non-PreviewKind values. Replace the `ext in KIND_BY_EXT` check with an own-property lookup using Object.hasOwn(KIND_BY_EXT, ext) or Object.prototype.hasOwnProperty.call(KIND_BY_EXT, ext) to ensure only direct properties of the KIND_BY_EXT object are considered.services/thumbnailer/README.md-7-14 (1)
7-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpecify a fenced code language for the flow diagram block.
Line 7 opens a fenced block without a language, which trips markdownlint MD040. Use a neutral language like
textfor the ASCII diagram.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/thumbnailer/README.md` around lines 7 - 14, The fenced code block in the README.md file containing the ASCII flow diagram (at line 7) is missing a language specifier, which violates markdownlint rule MD040. Add the language identifier `text` immediately after the opening triple backticks to specify that this is a text-based ASCII diagram, making the fenced block properly formatted.Source: Linters/SAST tools
🧹 Nitpick comments (1)
apps/library/components/mdx-view.tsx (1)
55-77: ⚡ Quick winAlign component styling with required theme token usage (
C.*).This TSX component styles preview surfaces via utility classes, but the required color token source is
C.*from@/lib/themefor component styling. Please switch surface/border/text color bindings toC.bg,C.textMuted,C.borderLight, etc., for compliance and consistency.As per coding guidelines, "Use CSS variables (C.bg, C.bgWhite, C.text, C.textMuted, C.accent, C.border, C.borderLight, C.headerBg, C.headerText) imported from
@/lib/themefor all styling; never hardcode colors except for subject palette colors."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/library/components/mdx-view.tsx` around lines 55 - 77, The mdx-view component is using Tailwind utility classes (bg-surface, border-line-soft, text-muted) for styling preview surfaces instead of CSS variables from `@/lib/theme`. Replace the className utilities with inline style objects using C.* tokens: replace bg-surface with C.bg, border-line-soft with C.borderLight, text-muted with C.textMuted, and border styling with appropriate C.* values. Apply these changes to all three div elements in the component (the loading state div, error state div, and content render div) to ensure consistent theming compliance throughout the file.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/library/app/api/pdf/route.ts`:
- Around line 56-113: Wrap the entire code block in a try/catch statement to
handle any thrown errors gracefully. For the Promise.all containing the
convex.query calls for api.library.files.get and
api.library.files.getDownloadUrl, add an AbortSignal with a timeout (e.g., 5-10
seconds) to both queries to prevent hanging. For both fetch calls (the one
accessing the stored PDF URL and the one calling the Gotenberg PDF service), add
an AbortSignal with explicit timeouts to the fetch requests. In the catch block,
return a Response with an appropriate error message and status code (e.g., 500
for internal errors or 504 for timeout errors) to prevent unhandled exceptions
from propagating.
In `@apps/library/app/globals.css`:
- Around line 56-79: The `.markdown-preview h1`, `.markdown-preview h2`,
`.markdown-preview h3`, and related heading styles do not comply with the
required MDX header specification. Update these selectors to include:
text-transform: uppercase on all headers, adjust font-size values to match the
mandated MDX scale (h1 should use text-3xl with md:text-4xl equivalent, h2 and
h3 with their respective sizes), apply letter-spacing equivalent to
tracking-tighter, set margin to 0 0 1.5rem 0 (mb-6), and ensure font-weight is
set to bold (700). Remove or adjust the border-bottom and padding-bottom
properties on h1 and h2 as needed to match the specification requirements.
In `@apps/library/app/internal/md-thumb/page.tsx`:
- Around line 79-81: The fetch call that retrieves src does not include a
timeout mechanism, which can cause requests to hang indefinitely if the upstream
server stalls. Modify the fetch invocation to include an AbortSignal with a
timeout by passing a signal option to the fetch call using
AbortSignal.timeout(...). This ensures the request will fail after a reasonable
duration, allowing the error to be caught by the existing catch block and
preventing resource exhaustion.
- Around line 68-73: Remove the `token` parameter from the searchParams
destructuring and validation logic in the page component. Instead, implement
header-based authentication by accessing the token from the request headers
(through Next.js request context or middleware) and validating it there. This
prevents the authentication secret from being exposed through URL query
parameters, which can be logged, cached by proxies, and retained in browser
history. Ensure the client-side code that calls this page also sends the token
via an HTTP header (such as Authorization) rather than as a query parameter.
In `@apps/library/components/markdown-preview.tsx`:
- Around line 99-101: The className in the markdown-preview.tsx component at the
button styling section hardcodes text-white for the active variant state instead
of using theme tokens. Replace the hardcoded text-white with an appropriate CSS
variable token imported from `@/lib/theme` (such as C.text or a similar foreground
color token). Ensure the active state styling when variant === v uses only
theme-driven color tokens from the required CSS variable imports, following the
coding guideline to never hardcode colors except for subject palette colors.
In `@apps/library/components/mdx-view.tsx`:
- Around line 26-33: The compileMdxAction(source) call at line 27 can be
rejected as a promise, but this rejection bypasses the "error" in result branch
check, leaving the component stuck in a loading state with an unhandled
rejection. Wrap the entire async operation (the await compileMdxAction(source)
call and the subsequent if block that checks for "error" in result) in a
try/catch block, and in the catch handler, set the error state with an
appropriate error message to ensure all failure modes are properly handled and
the component reflects the error state.
- Around line 34-37: The `run()` function call at line 34-37 in mdx-view.tsx
executes compiled MDX directly in the browser, creating a stored-XSS
vulnerability for user-uploaded content. To fix this, implement one of the
following: (1) Add authorization checks to ensure only trusted authors can
preview files before the `run()` call executes, (2) Sanitize the
`result.compiled` output by validating and removing any potentially dangerous
code patterns before passing it to `run()`, or (3) Replace the `run()` execution
with a trusted Markdown-only renderer that does not evaluate arbitrary code.
Verify that whichever approach is chosen prevents arbitrary code execution from
untrusted MDX sources.
In `@packages/backend/convex/library/files.ts`:
- Around line 225-234: The detail-page artifact URLs (plainThumbnailUrl, pdfUrl,
and darkPdfUrl) are being returned whenever their corresponding storage IDs
exist, without checking if the thumbnail regeneration is complete. This creates
a state mismatch where stale PDFs can be shown to users even after a backfill or
regeneration sets the status to pending or processing. Gate each of these three
URL fields on the same thumbnailStatus === "completed" condition used by
thumbnailUrlsFor() so they are only returned when the new render is actually
complete, preventing stale artifacts from being served during thumbnail
regeneration.
- Around line 461-475: The enqueueAllThumbnails function currently uses
collect() to load all libraryFiles at once, which can exceed Convex mutation and
read limits as the table grows. Replace the collect() call with take() using a
fixed batch size of 100-200 records (consistent with the codebase pattern), and
modify the handler to process only that batch. This allows the operation to
resume across multiple invocations instead of attempting to process the entire
table in a single mutation. The existing by_thumbnail_status index can
optionally be used to target only pending items more precisely if needed.
In `@packages/backend/convex/library/thumbnail_kind.ts`:
- Around line 27-33: The thumbnailKind function converts the contentType to
lowercase but does not remove MIME type parameters (the part after semicolon).
This causes MIME types with parameters like "text/markdown; charset=utf-8" to
fail the exact match checks. After converting contentType to lowercase in the ct
variable, split on the semicolon character and use only the first part (before
any parameters) for all subsequent MIME type classification checks including the
comparisons with "image/svg+xml", "text/markdown", "application/pdf", and the
startsWith checks.
In `@packages/backend/convex/library/thumbnails.ts`:
- Around line 49-56: The fetch call in the `callService` function (lines 49-56)
lacks a timeout mechanism, which could cause the Convex action to block
indefinitely if the render service stalls after accepting the connection. Add an
`AbortController` with a configurable timeout deadline by creating a new
AbortController instance before the fetch call, setting up a timeout that aborts
the controller after a specified duration (in milliseconds), and passing the
controller's signal to the fetch options object. This ensures the request fails
fast if the service is unresponsive, allowing it to reach the
`markThumbnailFailed` handler. Reference the existing timeout pattern used in
`agent_action.ts` as an example of how to implement this in the codebase.
In `@services/thumbnailer/docker-compose.yml`:
- Around line 30-31: The `depends_on` directive in the thumbnailer service only
enforces start order and does not wait for the gotenberg service to be fully
ready, causing early render failures. Add a healthcheck configuration to the
gotenberg service definition (around lines 36-45) that validates the
LibreOffice/Chromium endpoints are accessible, then modify the thumbnailer's
`depends_on` section (lines 30-31) to change the gotenberg dependency from a
simple list format to an object format with a `condition: service_healthy`
constraint to ensure the thumbnailer only starts after gotenberg has passed its
health check.
In `@services/thumbnailer/Dockerfile`:
- Around line 19-22: The Dockerfile runs the thumbnailer service as root by
default because there is no USER directive specified before the CMD instruction.
Add a USER directive before the CMD ["node", "server.mjs"] line to switch to a
non-root user for better security. You can use an existing `node` user or create
a dedicated non-root user, ensuring proper file ownership if needed.
In `@services/thumbnailer/server.mjs`:
- Around line 25-27: The PORT, TOKEN, and GOTENBERG_URL configuration at the
start of the file currently allows TOKEN to default to an empty string when
THUMBNAIL_TOKEN environment variable is not set. This disables authentication
check, creating a security vulnerability if the service becomes reachable
outside a private network. Instead of defaulting TOKEN to an empty string, the
service should fail closed by requiring THUMBNAIL_TOKEN to be set—either throw
an error or call process.exit() if the environment variable is missing. Apply
the same fail-closed pattern at the other two affected locations (around lines
276-279 and 344-347) where token validation likely occurs, ensuring that
authentication is always enforced and the service cannot operate without a valid
token configured.
- Around line 167-169: The .sort() method on the filtered page files array uses
lexicographic (alphabetical) sorting, which incorrectly orders filenames like
"p-10.png" before "p-2.png". Replace the default .sort() call with a custom
comparator function that extracts the numeric page number from each filename
(the digits between "p-" and ".png") and performs a numeric comparison on those
extracted numbers to ensure correct ordering regardless of page count.
- Line 23: The execFile calls for pdftoppm and img2pdf lack timeout and
maxBuffer protection, allowing malformed or expensive files to block worker
threads indefinitely. Create a shared wrapper function that wraps execFileP with
configurable timeout and maxBuffer options to protect against resource
exhaustion and hanging processes. Apply this wrapper function to all conversion
command invocations: both pdftoppm calls and the img2pdf call throughout the
file, ensuring each execution respects the timeout and maxBuffer limits.
- Around line 291-298: The code buffers entire responses into memory using
`Buffer.from(await fileRes.arrayBuffer())` before checking if they exceed
MAX_BYTES, allowing oversized fetches to consume memory before rejection.
Replace all instances where the response is fully buffered into memory with a
streaming approach that accumulates bytes incrementally from the response body
and aborts as soon as the accumulated size exceeds MAX_BYTES, preventing memory
exhaustion on large responses. This fix applies to both user-provided URL fetch
paths mentioned in the comment where `fileRes.arrayBuffer()` is called.
- Around line 68-72: The fetch() function in fetchWithTimeout() at
services/thumbnailer/server.mjs lines 68-72 follows HTTP redirects by default,
which bypasses the SSRF allowlist check performed by isAllowedUrl(). To fix
this, add redirect: 'manual' to the fetch options in the fetchWithTimeout
function to prevent following redirects. This same fix must also be applied at
services/thumbnailer/server.mjs lines 286-291 and
services/thumbnailer/server.mjs lines 352-357 where similar fetch calls occur
without redirect blocking.
---
Minor comments:
In `@apps/library/app/file/`[id]/page.tsx:
- Line 346: The span element that creates the PDF preview tooltip contains the
shadow-xl utility class on line 346 of apps/library/app/file/[id]/page.tsx,
which violates the coding guidelines that restrict shadow-xl usage to the Search
dialog only. Remove the shadow-xl class from the className attribute of this
tooltip span while keeping the other styling (border, border-line-soft,
bg-surface, and other utilities) intact.
In `@apps/library/components/file-preview.tsx`:
- Line 11: Replace hardcoded color utility classes with theme color variables
from `@/lib/theme` throughout the file. In the FRAME constant definition, replace
the hardcoded bg-white class with the appropriate theme variable such as
C.bgWhite. Apply the same approach to identify and replace any other hardcoded
white or black color utilities elsewhere in the file with their corresponding
theme color tokens (e.g., C.bg, C.text, C.textMuted, C.border, C.borderLight,
etc.) to ensure consistency with the design system and proper light/dark theme
support.
In `@apps/library/components/file-type-icon.tsx`:
- Around line 50-62: The COLORS constant in file-type-icon.tsx contains
hardcoded hex color values which violates the styling guidelines. Replace all
hardcoded hex values in the COLORS map with CSS variables imported from
`@/lib/theme` (such as C.bg, C.bgWhite, C.text, C.textMuted, C.accent, C.border,
C.borderLight, C.headerBg, C.headerText, or other appropriate theme variables).
First add the necessary import statement for the theme variables at the top of
the file, then update each color entry in the COLORS object to reference the
corresponding theme variable instead of its hardcoded hex equivalent.
In `@apps/library/lib/preview.ts`:
- Around line 75-77: The condition using the `in` operator for extension lookup
in the KIND_BY_EXT mapping checks the entire prototype chain, not just own
properties, which creates a security vulnerability where a crafted filename like
`file.__proto__` could return non-PreviewKind values. Replace the `ext in
KIND_BY_EXT` check with an own-property lookup using Object.hasOwn(KIND_BY_EXT,
ext) or Object.prototype.hasOwnProperty.call(KIND_BY_EXT, ext) to ensure only
direct properties of the KIND_BY_EXT object are considered.
In `@packages/backend/convex/schema.ts`:
- Around line 291-294: The comment describing the thumbnail lifecycle states
that office files are not thumbnailable (listed under the `none` state), but
this contradicts the current implementation where `thumbnailKind()` and the
generator both support Office thumbnails. Remove "office" from the list of file
types shown in the `none` lifecycle state comment to accurately reflect that
Office files are now supported by the thumbnail pipeline.
In `@services/thumbnailer/README.md`:
- Around line 7-14: The fenced code block in the README.md file containing the
ASCII flow diagram (at line 7) is missing a language specifier, which violates
markdownlint rule MD040. Add the language identifier `text` immediately after
the opening triple backticks to specify that this is a text-based ASCII diagram,
making the fenced block properly formatted.
---
Nitpick comments:
In `@apps/library/components/mdx-view.tsx`:
- Around line 55-77: The mdx-view component is using Tailwind utility classes
(bg-surface, border-line-soft, text-muted) for styling preview surfaces instead
of CSS variables from `@/lib/theme`. Replace the className utilities with inline
style objects using C.* tokens: replace bg-surface with C.bg, border-line-soft
with C.borderLight, text-muted with C.textMuted, and border styling with
appropriate C.* values. Apply these changes to all three div elements in the
component (the loading state div, error state div, and content render div) to
ensure consistent theming compliance throughout the file.
🪄 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 Plus
Run ID: a53c1bdb-d7df-4ee5-ab94-167898637c1d
⛔ Files ignored due to path filters (2)
packages/backend/convex/_generated/api.d.tsis excluded by!**/_generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
apps/library/app/actions/compile-mdx.tsapps/library/app/api/pdf/route.tsapps/library/app/file/[id]/page.tsxapps/library/app/globals.cssapps/library/app/internal/md-thumb/page.tsxapps/library/app/layout.tsxapps/library/components/file-card.tsxapps/library/components/file-preview.tsxapps/library/components/file-type-icon.tsxapps/library/components/markdown-preview.tsxapps/library/components/mdx-view.tsxapps/library/lib/mdx-components.tsxapps/library/lib/preview.tsapps/library/next.config.tsapps/library/package.jsonpackages/backend/convex.jsonpackages/backend/convex/library/files.tspackages/backend/convex/library/thumbnail_kind.tspackages/backend/convex/library/thumbnails.tspackages/backend/convex/schema.tspackages/backend/package.jsonservices/thumbnailer/.dockerignoreservices/thumbnailer/.env.exampleservices/thumbnailer/Dockerfileservices/thumbnailer/README.mdservices/thumbnailer/docker-compose.ymlservices/thumbnailer/package.jsonservices/thumbnailer/server.mjs
Comment on lines
+56
to
+113
| const [file, url] = await Promise.all([ | ||
| convex.query(api.library.files.get, { fileId: id }), | ||
| convex.query(api.library.files.getDownloadUrl, { fileId: id }), | ||
| ]); | ||
| if (!file || !url) { | ||
| return new Response("File not found", { status: 404 }); | ||
| } | ||
|
|
||
| const ct = file.contentType.toLowerCase(); | ||
| const isOffice = OFFICE_MIMES.has(ct); | ||
| const isMarkdown = ct === "text/markdown"; | ||
| const isPdf = ct === "application/pdf"; | ||
| // PDF files only support the dark variant (the original is the file itself). | ||
| if (!isOffice && !isMarkdown && !(isPdf && dark)) { | ||
| return new Response("This file type can't be converted to PDF", { | ||
| status: 415, | ||
| }); | ||
| } | ||
|
|
||
| const base = pdfName(file.originalName).replace(/\.pdf$/, ""); | ||
| const headers = { | ||
| "content-type": "application/pdf", | ||
| "content-disposition": contentDisposition( | ||
| `${base}${dark ? "-dark" : ""}.pdf`, | ||
| ), | ||
| "cache-control": "no-store", | ||
| }; | ||
|
|
||
| // Fast path: stream the pre-rendered blob (dark PDF, or styled/Office light PDF). | ||
| const stored = dark | ||
| ? file.darkPdfUrl | ||
| : style !== "plain" | ||
| ? file.pdfUrl | ||
| : null; | ||
| if (stored) { | ||
| const res = await fetch(stored); | ||
| if (res.ok) return new Response(res.body, { headers }); | ||
| } | ||
|
|
||
| // Fallback: convert on demand via the render service (Gotenberg). | ||
| const res = await fetch(`${serviceUrl.replace(/\/+$/, "")}/pdf`, { | ||
| method: "POST", | ||
| headers: { | ||
| "content-type": "application/json", | ||
| ...(token ? { "x-thumbnail-token": token } : {}), | ||
| }, | ||
| body: JSON.stringify({ | ||
| url, | ||
| contentType: file.contentType, | ||
| style, | ||
| theme: dark ? "dark" : "light", | ||
| }), | ||
| }); | ||
| if (!res.ok) { | ||
| return new Response(`Conversion failed (${res.status})`, { status: 502 }); | ||
| } | ||
| return new Response(res.body, { headers }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "route.ts" | grep -i pdfRepository: Wikipefia/wikipefia-ck
Length of output: 237
🏁 Script executed:
cat -n apps/library/app/api/pdf/route.ts | head -150Repository: Wikipefia/wikipefia-ck
Length of output: 4733
🏁 Script executed:
rg "fetchWithTimeout|AbortController" --type ts --type tsxRepository: Wikipefia/wikipefia-ck
Length of output: 94
🏁 Script executed:
rg "api/.*route\.ts" -l | head -5Repository: Wikipefia/wikipefia-ck
Length of output: 48
🏁 Script executed:
rg "try.*await.*fetch|fetch.*catch" -A 3 -B 1 --type ts --type tsx | head -50Repository: Wikipefia/wikipefia-ck
Length of output: 94
🏁 Script executed:
find apps -name "route.ts" -o -name "route.js"Repository: Wikipefia/wikipefia-ck
Length of output: 142
🏁 Script executed:
rg "export async function GET|export async function POST" --type ts -B 2 -A 15 | head -100Repository: Wikipefia/wikipefia-ck
Length of output: 1287
🏁 Script executed:
rg "AbortController|timeout" --type ts | head -20Repository: Wikipefia/wikipefia-ck
Length of output: 162
🏁 Script executed:
cat -n apps/library/app/api/uploadthing/route.tsRepository: Wikipefia/wikipefia-ck
Length of output: 365
🏁 Script executed:
rg "try\s*\{" apps --type ts -B 2 -A 8 | head -60Repository: Wikipefia/wikipefia-ck
Length of output: 3811
Harden upstream calls with explicit error handling and timeouts.
Convex queries (lines 56–59) and both fetch calls (lines 91, 96–108) can throw or hang without error handling. Wrap upstream calls in try/catch and apply bounded timeouts to prevent unhandled 500 errors and request exhaustion.
Suggested hardening pattern
+async function fetchWithTimeout(
+ input: RequestInfo | URL,
+ init: RequestInit = {},
+ timeoutMs = 15_000,
+) {
+ const controller = new AbortController();
+ const timer = setTimeout(() => controller.abort(), timeoutMs);
+ try {
+ return await fetch(input, { ...init, signal: controller.signal });
+ } finally {
+ clearTimeout(timer);
+ }
+}
+
export async function GET(req: NextRequest) {
+ try {
const params = req.nextUrl.searchParams;
// ...
- const [file, url] = await Promise.all([
+ const [file, url] = await Promise.all([
convex.query(api.library.files.get, { fileId: id }),
convex.query(api.library.files.getDownloadUrl, { fileId: id }),
]);
// ...
if (stored) {
- const res = await fetch(stored);
+ const res = await fetchWithTimeout(stored);
if (res.ok) return new Response(res.body, { headers });
}
- const res = await fetch(`${serviceUrl.replace(/\/+$/, "")}/pdf`, {
+ const res = await fetchWithTimeout(`${serviceUrl.replace(/\/+$/, "")}/pdf`, {
method: "POST",
headers: {
"content-type": "application/json",
...(token ? { "x-thumbnail-token": token } : {}),
},
body: JSON.stringify({ url, contentType: file.contentType, style, theme: dark ? "dark" : "light" }),
});
if (!res.ok) {
return new Response(`Conversion failed (${res.status})`, { status: 502 });
}
return new Response(res.body, { headers });
+ } catch {
+ return new Response("Upstream PDF service failure", { status: 502 });
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/library/app/api/pdf/route.ts` around lines 56 - 113, Wrap the entire
code block in a try/catch statement to handle any thrown errors gracefully. For
the Promise.all containing the convex.query calls for api.library.files.get and
api.library.files.getDownloadUrl, add an AbortSignal with a timeout (e.g., 5-10
seconds) to both queries to prevent hanging. For both fetch calls (the one
accessing the stored PDF URL and the one calling the Gotenberg PDF service), add
an AbortSignal with explicit timeouts to the fetch requests. In the catch block,
return a Response with an appropriate error message and status code (e.g., 500
for internal errors or 504 for timeout errors) to prevent unhandled exceptions
from propagating.
Comment on lines
+56
to
+79
| .markdown-preview h1, | ||
| .markdown-preview h2, | ||
| .markdown-preview h3, | ||
| .markdown-preview h4, | ||
| .markdown-preview h5, | ||
| .markdown-preview h6 { | ||
| font-family: var(--font-mono), sans-serif; | ||
| font-weight: 600; | ||
| line-height: 1.25; | ||
| margin: 1.6em 0 0.6em; | ||
| } | ||
| .markdown-preview h1 { | ||
| font-size: 1.7em; | ||
| border-bottom: 1px solid var(--c-border-light); | ||
| padding-bottom: 0.2em; | ||
| } | ||
| .markdown-preview h2 { | ||
| font-size: 1.4em; | ||
| border-bottom: 1px solid var(--c-border-light); | ||
| padding-bottom: 0.2em; | ||
| } | ||
| .markdown-preview h3 { | ||
| font-size: 1.2em; | ||
| } |
There was a problem hiding this comment.
MDX heading typography does not match the required header spec.
Lines 56–79 define h1/h2/h3 styles that miss the mandated uppercase + exact MDX header scale/spacing/tracking rules. Please align these selectors to the required h1–h4 style contract. As per coding guidelines, “MDX h1 headers: use text-3xl md:text-4xl tracking-tighter mb-6 uppercase font-bold…”, “MDX h2…”, “MDX h3…”, and “MDX h4…”, plus “Apply uppercase styling to all headers…”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/library/app/globals.css` around lines 56 - 79, The `.markdown-preview
h1`, `.markdown-preview h2`, `.markdown-preview h3`, and related heading styles
do not comply with the required MDX header specification. Update these selectors
to include: text-transform: uppercase on all headers, adjust font-size values to
match the mandated MDX scale (h1 should use text-3xl with md:text-4xl
equivalent, h2 and h3 with their respective sizes), apply letter-spacing
equivalent to tracking-tighter, set margin to 0 0 1.5rem 0 (mb-6), and ensure
font-weight is set to bold (700). Remove or adjust the border-bottom and
padding-bottom properties on h1 and h2 as needed to match the specification
requirements.
Source: Coding guidelines
Comment on lines
+99
to
+101
| className={`px-3 py-1 transition-colors ${ | ||
| variant === v ? "bg-accent text-white" : "text-muted hover:text-fg" | ||
| }`} |
There was a problem hiding this comment.
Avoid hardcoded text-white; use theme color tokens for state styling.
Line 100 hardcodes text-white for the active button state, which bypasses the required CSS-variable theme contract. Switch to token-driven foreground/background classes or inline CSS vars from the theme map. As per coding guidelines, “Use CSS variables (C.bg, C.bgWhite, C.text, C.textMuted, C.accent, C.border, C.borderLight, C.headerBg, C.headerText) imported from @/lib/theme for all styling; never hardcode colors except for subject palette colors.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/library/components/markdown-preview.tsx` around lines 99 - 101, The
className in the markdown-preview.tsx component at the button styling section
hardcodes text-white for the active variant state instead of using theme tokens.
Replace the hardcoded text-white with an appropriate CSS variable token imported
from `@/lib/theme` (such as C.text or a similar foreground color token). Ensure
the active state styling when variant === v uses only theme-driven color tokens
from the required CSS variable imports, following the coding guideline to never
hardcode colors except for subject palette colors.
Source: Coding guidelines
Comment on lines
+68
to
+72
| async function fetchWithTimeout(url, opts = {}) { | ||
| const controller = new AbortController(); | ||
| const timer = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); | ||
| try { | ||
| return await fetch(url, { ...opts, signal: controller.signal }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only check: inspect service docs/env for expected storage host/redirect behavior assumptions.
fd -a 'README.md|\.env.example|docker-compose.yml' \
| xargs -r sed -n '/ALLOWED_HOSTS/p;/THUMBNAIL/p;/convex/p;/redirect/p'Repository: Wikipefia/wikipefia-ck
Length of output: 2104
🏁 Script executed:
# Inspect the actual fetchWithTimeout implementation and surrounding context
head -100 services/thumbnailer/server.mjs | tail -50Repository: Wikipefia/wikipefia-ck
Length of output: 1557
🏁 Script executed:
# Find and read the isAllowedUrl function
grep -n "isAllowedUrl" services/thumbnailer/server.mjs | head -5Repository: Wikipefia/wikipefia-ck
Length of output: 162
🏁 Script executed:
# Check the full structure of server.mjs to understand URL validation flow
wc -l services/thumbnailer/server.mjsRepository: Wikipefia/wikipefia-ck
Length of output: 102
🌐 Web query:
Convex Storage API redirect behavior files download
💡 Result:
In Convex, file storage retrieval behavior depends on the method used to access the file [1][2]. 1. Using storage.getUrl: When you call ctx.storage.getUrl(storageId), Convex generates a URL that provides direct access to the file [1][3]. While the exact underlying mechanism can involve internal redirects to highly available storage, this is the standard, optimized method for serving files to clients (e.g., in img tags or for direct downloads) [1][3][4]. It is recommended for most use cases, including large files, as it offloads the serving process [1][5]. 2. Using HTTP Actions: If you require custom access control, you can serve files via HTTP actions using ctx.storage.get(storageId) [1][5]. - In this pattern, the action fetches the file content (returning a Blob) and serves it directly in the HTTP response [1][6]. - Important Limitation: HTTP action responses have a size limit (currently 20MB) [1][5]. For files exceeding this size, you should use storage.getUrl() to generate a direct URL and, if necessary, perform a 302 redirect from your HTTP action to that URL [1][5]. 3. Redirect Behavior in Ecosystem Tools: Some community-built packages, such as convex-fs or static-hosting, specifically implement redirect-based patterns [7][8]. For example, these tools may serve a 302 redirect to a CDN edge URL (such as Bunny.net) to cache and accelerate file delivery [7][8]. This behavior is a feature of those specific integrations and not the default behavior of the core storage.getUrl() method [1][7][8]. In summary, for standard file serving, prefer storage.getUrl(). For custom access-controlled endpoints, use HTTP actions, but be mindful of the 20MB response limit and redirect to a getUrl() result for larger files [1][5].
Citations:
- 1: https://docs.convex.dev/file-storage/serve-files
- 2: https://docs.convex.dev/file-storage
- 3: https://docs.convex.dev/api/interfaces/server.StorageReader
- 4: https://docs.convex.dev/api/interfaces/server.StorageWriter.md
- 5: https://masteringconvex.com/storage/serve-files
- 6: https://docs.convex.dev/api/interfaces/server.StorageActionWriter.md
- 7: https://convexfs.dev/guides/serving-files/
- 8: https://github.com/get-convex/static-hosting/blob/main/INTEGRATION.md
🏁 Script executed:
# Check how Convex Storage URLs are used in the codebase
rg "convex\.cloud|ALLOWED_HOSTS" -A 3 -B 3Repository: Wikipefia/wikipefia-ck
Length of output: 3270
🏁 Script executed:
# Look for any actual file fetches in render/pdf endpoint handlers
grep -n "fetchWithTimeout\|fetch" services/thumbnailer/server.mjs | head -20Repository: Wikipefia/wikipefia-ck
Length of output: 1134
🏁 Script executed:
# Check if there are any examples or tests showing ALLOWED_HOSTS configuration with redirect-using domains
rg "ALLOWED_HOSTS|convex\.cloud" services/thumbnailer/ -B 2 -A 2Repository: Wikipefia/wikipefia-ck
Length of output: 1612
🏁 Script executed:
# Check if there's any indication that redirect-based storage or CDN is used
grep -r "redirect\|cdn\|edge" services/thumbnailer/ --include="*.md" --include="*.mjs" --include="*.yml"Repository: Wikipefia/wikipefia-ck
Length of output: 631
Block fetch redirects to prevent SSRF bypass.
The isAllowedUrl() validation checks only the initial URL, but fetch() follows 3xx redirects by default. If an allowed host redirects to an internal or disallowed URL, the SSRF allowlist is bypassed. For default configurations using convex.cloud, this risk is low since Convex Storage URLs do not redirect, but custom ALLOWED_HOSTS configurations with redirect-based storage or CDN providers would be vulnerable.
Proposed fix
async function fetchWithTimeout(url, opts = {}) {
const controller = new AbortController();
const timer = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS);
try {
- return await fetch(url, { ...opts, signal: controller.signal });
+ const res = await fetch(url, {
+ ...opts,
+ redirect: "manual",
+ signal: controller.signal,
+ });
+ if (res.status >= 300 && res.status < 400) {
+ const location = res.headers.get("location");
+ throw new Error(`redirects are not allowed${location ? `: ${location}` : ""}`);
+ }
+ return res;
} finally {
clearTimeout(timer);
}
}Also applies to: 286-291, 352-357
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/thumbnailer/server.mjs` around lines 68 - 72, The fetch() function
in fetchWithTimeout() at services/thumbnailer/server.mjs lines 68-72 follows
HTTP redirects by default, which bypasses the SSRF allowlist check performed by
isAllowedUrl(). To fix this, add redirect: 'manual' to the fetch options in the
fetchWithTimeout function to prevent following redirects. This same fix must
also be applied at services/thumbnailer/server.mjs lines 286-291 and
services/thumbnailer/server.mjs lines 352-357 where similar fetch calls occur
without redirect blocking.
- Add timeouts, auth hardening, and capped fetches for render services - Paginate thumbnail backfills and hide stale generated artifacts until ready - Improve MDX thumbnail auth and FileCard title/rating layout handling
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/thumbnailer/server.mjs (1)
116-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedirect following still allows SSRF bypass.
The past comment about blocking redirects remains unaddressed.
fetch()follows 3xx redirects by default, so an allowed host that issues a redirect to an internal/disallowed URL bypassesisAllowedUrl(). While Convex storage URLs typically don't redirect, customALLOWED_HOSTSconfigurations could be vulnerable.Proposed fix
async function fetchWithTimeout(url, opts = {}) { const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); try { - return await fetch(url, { ...opts, signal: controller.signal }); + const res = await fetch(url, { + ...opts, + redirect: "manual", + signal: controller.signal, + }); + if (res.status >= 300 && res.status < 400) { + const location = res.headers.get("location"); + throw new Error(`redirects are not allowed${location ? `: ${location}` : ""}`); + } + return res; } finally { clearTimeout(timer); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/thumbnailer/server.mjs` around lines 116 - 124, The fetchWithTimeout function does not prevent HTTP redirects, which allows an SSRF bypass where an allowed URL can redirect to a disallowed internal URL. To fix this, add a redirect option to the fetch call within the fetchWithTimeout function to disable automatic redirect following. Set the redirect property to 'error' in the fetch options (merge it with the existing opts spread) so that any 3xx redirect response will be treated as an error rather than followed automatically, ensuring that only the validated URL is actually accessed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@services/thumbnailer/server.mjs`:
- Around line 116-124: The fetchWithTimeout function does not prevent HTTP
redirects, which allows an SSRF bypass where an allowed URL can redirect to a
disallowed internal URL. To fix this, add a redirect option to the fetch call
within the fetchWithTimeout function to disable automatic redirect following.
Set the redirect property to 'error' in the fetch options (merge it with the
existing opts spread) so that any 3xx redirect response will be treated as an
error rather than followed automatically, ensuring that only the validated URL
is actually accessed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 104e2110-cf32-4687-8c8f-2be70fdab098
📒 Files selected for processing (10)
apps/library/app/api/pdf/route.tsapps/library/app/internal/md-thumb/page.tsxapps/library/components/file-card.tsxapps/library/components/mdx-view.tsxpackages/backend/convex/library/files.tspackages/backend/convex/library/thumbnail_kind.tspackages/backend/convex/library/thumbnails.tsservices/thumbnailer/Dockerfileservices/thumbnailer/docker-compose.ymlservices/thumbnailer/server.mjs
✅ Files skipped from review due to trivial changes (1)
- services/thumbnailer/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/library/components/mdx-view.tsx
- services/thumbnailer/docker-compose.yml
- apps/library/app/internal/md-thumb/page.tsx
- apps/library/components/file-card.tsx
- packages/backend/convex/library/files.ts
- apps/library/app/api/pdf/route.ts
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
1 participant
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.
Summary
Testing
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation