Conversation
|
@tomaszantas is attempting to deploy a commit to the langfuse Team on Vercel. A member of the Team first needs to authorize it. |
|
@claude pls review |
|
@jannikmaierhoefer can u pls approve the dev deployment? |
|
@claude pls review |
felixkrrr
left a comment
There was a problem hiding this comment.
Thanks for looking into this — the md-to-pdf endpoint is definitely broken in production and the Chromium bundling fixes are solid work. A few things need to be addressed before we can merge.
The core issue
langfusePathToMdSrcPath() patches one consumer (the md-to-pdf API route), but the underlying problem affects all .md URL access — the "Copy as Markdown" button, direct .md URLs, and content negotiation are all broken for the same pages.
We tested production and found 30 broken .md URLs:
23 marketing pages (all except /support.md which has a one-off beforeFiles rewrite):
/about.md, /careers.md, /cn.md, /community.md, /cookie-policy.md, /enterprise.md, /find-us.md, /imprint.md, /jp.md, /jp-cloud.md, /kr.md, /non-profit.md, /oss-friends.md, /press.md, /pricing.md, /pricing-self-host.md, /privacy.md, /research.md, /startups.md, /talk-to-us.md, /terms.md, /watch-demo.md, /wrapped.md
7 user story pages (URL path is /users/* but md-src/ has them under customers/):
/users.md, /users/canva.md, /users/cresta.md, /users/khan-academy.md, /users/magic-patterns-ai-design-tools.md, /users/merckgroup.md, /users/sumup.md
The root cause is in scripts/copy_md_sources.js — it copies files preserving the content/ directory structure, but the URL structure differs for these sections:
| Content path | md-src/ path (current) |
URL path | Result |
|---|---|---|---|
content/marketing/terms.mdx |
md-src/marketing/terms.md |
/terms.md → rewrite → md-src/terms.md |
404 |
content/customers/canva.mdx |
md-src/customers/canva.md |
/users/canva.md → rewrite → md-src/users/canva.md |
404 |
Suggested fix
Fix copy_md_sources.js to copy files to paths matching the URL structure, not the content directory:
content/marketing/terms.mdx→md-src/terms.mdcontent/customers/canva.mdx→md-src/users/canva.md
The mapping from content directory → URL path already exists in lib/source.ts (marketing baseUrl: "", customers baseUrl: "/users"), so the copy script should use the same source of truth.
This way the existing generic rewrite /:path*.md → /md-src/:path*.md works for all consumers without any remapping code. langfusePathToMdSrcPath(), lib/marketing-slugs.ts, and the one-off /support.md beforeFiles rewrite all become unnecessary.
What to keep
outputFileTracingIncludesfor Chromium binaries innext.config.mjs— this is correct and necessary.- Improved error handling/logging for
chromium.executablePath()and theSPARTICUZ_CHROMIUM_BIN_DIRoverride — solid additions.
What to change/remove
tsconfig.tsbuildinfo— remove from the commit, it's a 1.4MB build artifact.langfusePathToMdSrcPath()andlib/marketing-slugs.ts— should become unnecessary once the copy script is fixed.- The
/support.mdbeforeFiles rewrite already on main — can be removed as part of this fix.
|
Correction to my review above: The "Copy as Markdown" button only exists on docs-style pages (docs, self-hosting, integrations, guides, library) — not on marketing pages or user stories. So the broken The practical impact of the 30 broken
The rest of the review stands as-is. |
|
To clarify the above — here's why fixing this inside the API route isn't the right layer. The This PR adds
If we fix (Correction to my earlier review: the "Copy as Markdown" button only exists on docs-style pages, not marketing/user-story pages, so it's not directly affected here.) |
Description
Fix
api/md-to-pdf.