feedback on academy draft#2885
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
the
size:XL
|
@claude review |
dosubot
Bot
added
size:L
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91739921f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| The value of the loop is cumulative. Each step you add gives you better signal, more systematic coverage, and more confidence in what you are shipping. The goal is not to implement everything at once — it is to understand where you are and take the next step toward closing the loop. | ||
|
|
||
| {/* TODO: Link blog article about patterns of AI engineering lifecycle adoption once written */} | ||
| {/_ TODO: Link blog article about patterns of AI engineering lifecycle adoption once written _/} |
There was a problem hiding this comment.
The line {/_ TODO: ... _/} is parsed as a JavaScript expression in MDX (a RegExp literal), not as a comment, so the page tries to render a non-renderable object as a React child. This can break rendering for /academy/ai-engineering-loop at runtime/build time instead of showing content. Replace it with a valid MDX comment ({/* ... */}) or remove it.
Useful? React with 👍 / 👎.
| @@ -53,3 +61,5 @@ When monitoring surfaces something worth investigating, you have a few options: | |||
| - [Error analysis](/academy/error-analysis): how to look at traces in detail | |||
There was a problem hiding this comment.
This link points users to /academy/error-analysis, but this commit does not add a corresponding Academy page/route, so the navigation path leads to a 404. Because this appears in a “What comes next” section, readers are likely to hit the broken path during normal progression through the new Academy docs.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| style={{ maxHeight: "200px", width: "auto" }} | ||
| /> | ||
|
|
||
| TODO: please use standard image embed via markdown image, thereby the resizing onclick works |
There was a problem hiding this comment.
🔴 This PR adds ~17 plain-text TODO:/todo: paragraphs and one <Callout>TODO: Vibe patch and error analyse before</Callout> (experiments.mdx:54) across all 7 academy pages. These are bare markdown paragraphs and a styled component — not MDX comments — so they will render verbatim on the live docs site (including typos like "isolidation", "agemts", "refernce", "distribition", "repetetive", "differnt", "lable", "analyis"). Convert each note to an MDX comment {/* ... */} or move the feedback into PR review comments before merging.
Extended reasoning...
Bug
In MDX, anything that is not wrapped in {/* ... */} (JSX comment) or <!-- ... --> (HTML comment) and is not inside a non-rendering JSX prop is rendered. The PR adds many lines of the form TODO: ... / todo: ... directly in the body of the academy .mdx files, and one <Callout>TODO: Vibe patch and error analyse before</Callout>. These are plain markdown paragraphs (and one styled callout), not comments. After the broken JSX-comment delimiter on ai-engineering-loop.mdx ({/_ ... _/}) is fixed and the Vercel build succeeds, every one of these TODO notes will publish as visible body content on the live academy pages.
Locations confirmed
content/academy/ai-engineering-loop.mdx— lines 26, 36, 42, 54, 60, 68, 78, 86 (8 TODO paragraphs)content/academy/datasets.mdx—todo: this does not make sense...,todo: needs to have the right size...,TODO: it is a json field...,TODO: needs link to "reference-free evaluator" sectioncontent/academy/evaluate.mdx—todo: most production applications will require continuous labeling...,todo: this is a circular refernce...,todo: add link to error analyis blog here,todo: delete table...,todo: put this at the top...content/academy/experiments.mdx— line 11TODO: as mentioned on the index page, I dislike the idea here of forcing the isolidation of changes...and line 54<Callout>TODO: Vibe patch and error analyse before</Callout>content/academy/monitoring.mdx—todo: tracing needs a link back...,todo: read docs and make this page include more detail...,todo: call it evaluation and not quality scores...,TODO: refer to this as implicit user feedback signals...,todo: error analysis page does not exist in academy yetcontent/academy/tracing.mdx—TODO: refer to traditional observability here...,TODO: this does not introduce that a trace is...,TODO: instead of being this detailed here...,TODO: most common ones are ... (missing text),TODO: this rule here does not make sense...,TODO: already give error analysis a shoutout here?
The Callout at experiments.mdx:54 is particularly visually prominent because it inherits the styled box treatment used elsewhere on these pages. (Note: the Callout itself was pre-existing — this PR only collapsed it from multi-line to single-line — but the PR touches the line so it is the natural moment to remove it.)
Step-by-step proof
- Open
content/academy/ai-engineering-loop.mdx:26. The line isTODO: please use standard image embed via markdown image, thereby the resizing onclick works. - The line above (25) is the closing
/>of an<img />JSX element followed by a blank line; the line below (27) is also blank. - By MDX spec, a non-empty line of plain text separated from neighbours by blank lines is a markdown paragraph and is rendered as
<p>...</p>in the resulting HTML. There is no{/* */}wrapper, no<!-- -->, no surrounding JSX expression. - The Next.js/Nextra MDX pipeline used by this site renders the page; the paragraph would appear on the published academy page exactly as written, typos included.
- The same logic applies line-by-line to every other
TODO:/todo:listed above. - For
experiments.mdx:54,<Callout>TODO: Vibe patch and error analyse before</Callout>is valid MDX/JSX. TheCalloutcomponent is in scope (used elsewhere in the same PR) and renders its children inside a styled box, so the literal text would appear as a prominent visual element on/academy/experiments.
Impact
If merged as-is (after the JSX-comment build error is resolved), the live academy pages would publish ~17 internal feedback notes containing typos and meta-commentary like "I would mention here some details already...", "i would highlight that AI applications/agemts have many things...", "see our meeting notes". The Callout would render as a styled box reading "TODO: Vibe patch and error analyse before". This is plainly not intended for production readers — the PR title is "feedback on academy draft" and the notes are clearly directed at the document author.
Fix
Either (a) wrap each note in {/* ... */} so it does not render, (b) move the notes into PR review comments instead of file content, or (c) address the feedback inline and delete the TODO. Option (b) is the most natural given the PR title and intent.
| The value of the loop is cumulative. Each step you add gives you better signal, more systematic coverage, and more confidence in what you are shipping. The goal is not to implement everything at once — it is to understand where you are and take the next step toward closing the loop. | ||
|
|
||
| {/* TODO: Link blog article about patterns of AI engineering lifecycle adoption once written */} | ||
| {/_ TODO: Link blog article about patterns of AI engineering lifecycle adoption once written _/} |
There was a problem hiding this comment.
🔴 Line 76 of content/academy/ai-engineering-loop.mdx was changed from a valid JSX comment {/* TODO: ... */} to {/_ TODO: ... _/} (asterisks turned into underscores, likely by Prettier interpreting the inner * as Markdown emphasis when re-formatting). Inside an MDX/JSX expression {…}, /_ ... _/ is not a valid JS expression, so the page either fails to compile or renders the comment as visible body text — this is consistent with the failed Vercel deployment on this PR. Fix by restoring the {/* ... */} delimiters.
Extended reasoning...
What is broken
Line 76 of content/academy/ai-engineering-loop.mdx reads:
{/_ TODO: Link blog article about patterns of AI engineering lifecycle adoption once written _/}The original (pre-PR) value was a valid JSX comment:
{/* TODO: Link blog article about patterns of AI engineering lifecycle adoption once written */}The asterisks were replaced with underscores. The most likely cause is Prettier running in markdown mode and treating the inner * characters as Markdown emphasis markers, then escaping them to _ on re-format.
Why this breaks the build / page
In MDX, { ... } at the block level opens a JSX expression that is parsed as JavaScript by acorn. JSX-style comments work specifically because {/* ... */} parses as a { followed by a JS block-comment, which has no value, and the closing } — so the whole expression becomes an empty/no-op child and renders nothing.
With asterisks replaced by underscores, the expression body becomes /_ TODO: Link blog article about patterns of AI engineering lifecycle adoption once written _/. This is not valid JavaScript:
- It is not a comment (
//or/* */). - It is not a valid regex literal in this position — even if the parser tried to treat
/_..._/as a regex, the surrounding context expects an expression that produces a value rendered as a child, and the body contains spaces, colons, and reserved words that would still leave the surrounding JSX malformed. Some MDX parsers will hard-fail; others may parse/_..._/as a regex literal and stringify the resulting RegExp object as JSX children, dumping the literal text onto the page.
Either way, the intended invisible TODO comment is gone.
Evidence this is the cause of the failed deployment
- The PR timeline shows the Vercel deployment status as
Error/FAILEDfor this commit. - A repo-wide grep for
\{/_.*_/\}matches only this file — every other JSX comment in the repo (e.g.content/docs/prompt-management/features/link-to-traces.mdx,content/blog/2026-02-16-prompt-improvement-claude-skills.mdx) uses the canonical{/* ... */}form. - No other change in this PR touches code that the MDX compiler would reject; the rest of the diff is text edits, TODO notes, and Prettier whitespace/table reformatting.
Step-by-step proof
- MDX encounters
{/_ TODO: Link blog article ... once written _/}at block level. - The expression body is handed to acorn as JS.
- acorn sees
/_and either errors (no valid token sequence following), or parses/_ ... _/as a regex literal. - If it errors, the build fails — matches the observed Vercel
Errorstatus. - If it succeeds as a regex, the resulting
RegExpis returned as the JSX child for that expression slot, and React/Next renders it as/_ TODO: Link blog article about patterns of AI engineering lifecycle adoption once written _/on the published page. - In both branches, the original behavior (an invisible TODO note) is lost.
Fix
Restore the asterisks on line 76:
{/* TODO: Link blog article about patterns of AI engineering lifecycle adoption once written */}If Prettier in markdown mode keeps mangling this, an <!-- ... --> HTML comment outside any JSX expression context, or an inline-code escape, are safe alternatives — but the simplest fix is just putting the *s back.
| @@ -8,18 +8,20 @@ description: How to use experiments to systematically improve your LLM applicati | |||
|
|
|||
| To systematically understand and improve how your system behaves, you need a way to isolate cause and effect. That's what experiments give you. You pick one variable, run your dataset through two versions of your system, and compare what comes out. The result tells you whether a change actually helped, and by how much. To understand by how much, you also need to evaluate your experiment output (see evalaute TODO). This section covers the systematic experimentation part before evaluation. | |||
There was a problem hiding this comment.
🟣 Line 9 of content/academy/experiments.mdx has a typo "evalaute" (should be "evaluate") and an unresolved TODO placeholder where a link to /academy/evaluate was intended — both will publish verbatim to readers. This is a pre-existing issue (the diff shows the line as unchanged context), but worth fixing in this PR since it's actively iterating on the academy draft. Suggested fix: replace (see evalaute TODO) with (see [Evaluate](/academy/evaluate)).
Extended reasoning...
The bug. content/academy/experiments.mdx line 9 reads:\n\n> ... To understand by how much, you also need to evaluate your experiment output (see evalaute TODO). This section covers the systematic experimentation part before evaluation.\n\nTwo separate issues coexist on this single sentence, both visible to readers of the rendered page:\n\n1. Typo. evalaute is a misspelling of evaluate. The same paragraph spells it correctly two clauses earlier ("...you also need to evaluate your experiment output..."), so the inconsistency is glaring rather than systemic. There is no MDX/JSX construct that would suppress this from the output — it sits in plain body prose.\n\n2. Unresolved placeholder. The fragment (see evalaute TODO) is a writer's note meant to be replaced with an inline link to the Evaluate page (/academy/evaluate, which exists in this same PR). Because it is bare prose with no {/* */} MDX comment wrapper and no <Callout> styling, the literal string see evalaute TODO will render as part of the paragraph in the published academy.\n\nHow it manifests. The line is body prose inside the page's intro paragraph. Next.js/MDX will render it verbatim. Anyone landing on /academy/experiments sees a sentence that reads as if the author forgot to finish writing it.\n\nWhy other code does not prevent this. There is no spell-check or link-lint step in the pipeline that would catch either issue, and the bug is not inside an MDX comment block ({/* ... */}) — those are correctly used elsewhere in the same diff. The verifier confirmations note that even if the standalone TODO: lines and <Callout>TODO: ...</Callout> blocks elsewhere in this PR were stripped, this inline placeholder would survive because it lives mid-sentence in body prose.\n\nStep-by-step proof.\n\n1. Open content/academy/experiments.mdx at line 9 in this PR's tree.\n2. Read the second sentence of the second paragraph: it contains both "evaluate" (correctly spelled) and "evalaute" (misspelled), confirming issue (1).\n3. The same sentence ends with (see evalaute TODO) — note the absence of {/* */} wrapping and the presence of a closing parenthesis treating the phrase as part of the sentence, confirming issue (2).\n4. Run pnpm dev (or build) and visit /academy/experiments. The string (see evalaute TODO) appears verbatim in the rendered HTML.\n5. Compare with /academy/evaluate — that page exists in this PR, confirming the link target the author meant to write.\n\nSeverity rationale (pre_existing). The unified diff hunk @@ -8,18 +8,20 @@ shows line 9 with a leading space (context), not a + (addition), so the typo and placeholder existed before this PR. This PR is, however, explicitly the "feedback on academy draft" pass and edits the same file extensively (adding TODOs at lines 11, 51, etc.), so addressing it here is the natural moment.\n\nSuggested fix.\n\ndiff\n-The result tells you whether a change actually helped, and by how much. To understand by how much, you also need to evaluate your experiment output (see evalaute TODO). This section covers the systematic experimentation part before evaluation.\n+The result tells you whether a change actually helped, and by how much. To understand by how much, you also need to evaluate your experiment output (see [Evaluate](/academy/evaluate)). This section covers the systematic experimentation part before evaluation.\n
| In order to make it easy to differentiate between operations, you'll see different [types of observations](/docs/observability/features/observation-types). The most common ones are. Each type of observation is used to capture different kinds of interactions of an agent. | ||
|
|
||
| | Action of an agent | Typical observation data | | ||
| | --- | --- | | ||
| | A call to a language model | Full prompt or message history as input, the completion as output, plus metadata like the model name and token counts | | ||
| | A step that fetches information from an external source | Query and the returned documents | | ||
| | An invocation of a tool or function by an agent | Which tool was called, the arguments, and the return value | | ||
| | General processes | Highly dependent on the use case | | ||
| TODO: most common ones are ... (missing text) |
There was a problem hiding this comment.
🟣 Pre-existing issue — contains a truncated sentence: "The most common ones are." which ends mid-thought before the table of observation types. The author already flagged this in the adjacent line 48 TODO ("most common ones are ... (missing text)"), so the missing list (likely GENERATION, SPAN, EVENT, etc.) needs to be filled in before this draft ships.
Extended reasoning...
What the bug is
In content/academy/tracing.mdx line 46, the body prose reads:
In order to make it easy to differentiate between operations, you'll see different types of observations. The most common ones are. Each type of observation is used to capture different kinds of interactions of an agent.
The middle sentence is grammatically truncated — "The most common ones are." is a sentence fragment that was clearly meant to introduce a list of observation types but the list itself was never written. The text then jumps to "Each type of observation is used to capture..." as if the introduction had been completed.
How it manifests
This is regular body copy in an MDX file, so the broken sentence renders verbatim to readers as a published Academy page. Unlike the adjacent line 48 (TODO: most common ones are ... (missing text)) — which is plain text that will also render but at least signals "unfinished" to a reader — line 46 reads as if it were intentional prose. A reader hitting this page will see what looks like a typo or missing word, immediately followed by a table that the truncated sentence was clearly meant to introduce.
Why this is a pre-existing issue (severity rationale)
Looking at the diff hunk for tracing.mdx around line 46, the truncated sentence appears in unchanged context (no + or - prefix on that line). What this PR adds nearby is the TODO note on line 48 and reformatted whitespace in the table below — but the truncation itself existed in main before this PR. So technically this is pre_existing.
That said, this PR is explicitly review feedback on the academy draft, and the author called out exactly this gap with their own TODO directly underneath the broken sentence. So while the strict severity is pre_existing, it is squarely in-scope feedback for this review and should be fixed before the academy pages ship.
Step-by-step proof
- Open
content/academy/tracing.mdx. - Navigate to line 46 inside the "### Observation types" section.
- Read the paragraph: "In order to make it easy to differentiate between operations, you'll see different types of observations. The most common ones are. Each type of observation is used to capture different kinds of interactions of an agent."
- Note that "The most common ones are." is followed by a period, not a colon or list — the sentence is grammatically complete but semantically empty ("are" what?).
- Confirm by reading line 48:
TODO: most common ones are ... (missing text)— the author's own note acknowledging the omission. - Confirm impact: this prose renders as published page content at
/academy/tracing.
How to fix
Replace "The most common ones are." with the actual list the author intended, and remove the line 48 TODO once filled in. For example:
The most common ones are generation, span, event, and retrieval. Each type of observation is used to capture different kinds of interactions of an agent.
(Reference /docs/observability/features/observation-types for the canonical list to make sure the names match the docs.)
| - [Error analysis](/academy/error-analysis): how to look at traces in detail | ||
| - [Datasets](/academy/datasets): capturing production traces for evaluation | ||
| - [Experiments](/academy/experiments): testing whether a fix actually worked | ||
|
|
||
| todo: error analysis page does not exist in academy yet |
There was a problem hiding this comment.
🟣 Pre-existing: the link Error analysis on monitoring.mdx:61 will 404 — content/academy/ has no error-analysis.mdx (only ai-engineering-loop, datasets, evaluate, experiments, index, monitoring, tracing). The diff itself doesn't introduce the broken link, but this PR adds a TODO on line 65 acknowledging the gap, so it's a good moment to either ship the page or remove/redirect the link before merge.
Extended reasoning...
What's wrong
content/academy/monitoring.mdx line 61 contains - [Error analysis](/academy/error-analysis): how to look at traces in detail inside the "What comes next" section. Listing content/academy/ shows the directory contains only ai-engineering-loop.mdx, datasets.mdx, evaluate.mdx, experiments.mdx, index.mdx, meta.json, monitoring.mdx, and tracing.mdx. There is no error-analysis.mdx, so the link target does not exist.
Why it manifests
The Langfuse docs site is built with Nextra, which resolves /academy/error-analysis to a page under content/academy/. With no matching MDX file, the route returns a 404. Because the link is rendered in the "What comes next" navigation footer of the monitoring page — exactly where readers are pointed after finishing the lesson — this is one of the more visible places to break.
Pre-existing vs. introduced
Looking at the diff for monitoring.mdx, the line - [Error analysis](/academy/error-analysis): how to look at traces in detail appears as unchanged context (no +/- prefix). What this PR does add, on line 65, is the acknowledgement todo: error analysis page does not exist in academy yet. So strictly speaking the broken link pre-dates this PR — hence the pre_existing severity — but the PR explicitly draws attention to the gap and is editing this exact area, which makes it the natural place to resolve.
Step-by-step proof
- Open
content/academy/monitoring.mdxand read line 61:- [Error analysis](/academy/error-analysis): how to look at traces in detail. - List
content/academy/:ai-engineering-loop.mdx,datasets.mdx,evaluate.mdx,experiments.mdx,index.mdx,meta.json,monitoring.mdx,tracing.mdx. Noerror-analysis.mdx. - Nextra maps
/academy/<slug>tocontent/academy/<slug>.mdx. Sinceerror-analysis.mdxis absent, requesting/academy/error-analysisreturns a 404. - Confirmed by the PR author themselves on line 65:
todo: error analysis page does not exist in academy yet.
How to fix
Either ship a stub content/academy/error-analysis.mdx in this PR (and add it to meta.json), or remove the bullet on line 61 (and the related Learn more here: TODO insert link reference around line 49) until the page is ready. A redirect entry could also work if the topic is currently covered elsewhere.
1 participant
Disclaimer: Experimental PR review
Greptile Summary
This PR introduces a new Langfuse Academy section — a complete set of educational pages covering the AI engineering lifecycle (tracing, monitoring, datasets, experiments, evaluation) — along with all the necessary routing, navigation, and source configuration to wire it into the live site.
Because the full routing infrastructure is live in this PR, the draft content will be publicly accessible. There are two blocking issues to resolve before merging:
{/_ ... _/}onai-engineering-loop.mdxline 152 is a malformed MDX comment (underscores instead of asterisks) that will cause a React render error and break that page entirely.Confidence Score: 3/5
Not safe to merge — draft TODO text will be publicly visible and a malformed MDX comment will break a page at runtime.
Two P1 issues: the invalid
{/_ _/}MDX comment causes a React render error on the core overview page, and pervasive inline TODO comments across all content files will be visible to users on the live site. The infrastructure additions (routing, source, registry, nav) are clean and consistent with existing patterns.All six
content/academy/*.mdxfiles need TODO comments resolved;ai-engineering-loop.mdxneeds the MDX comment syntax fixed;monitoring.mdxneeds the broken/academy/error-analysislink removed or replaced.Important Files Changed
{/_ ... _/}) that will throw a React render error at runtime./academy/error-analysiswhich doesn't exist.<Callout>TODO: Vibe patch…</Callout>block that renders TODO text in a styled callout box.academySource; follows same pattern as other sections in the codebase.academySourceloader following the same pattern ashandbookSourceand others; clean addition.academysection withlayout: "docs"andhasOwnRoute: true; consistent with other sections.academycollection usingsidebarFrontmatterSchema, consistent with howhandbookis defined.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[source.config.ts\ndefineDocs academy] --> B[lib/source.ts\nacademySource loader] B --> C[lib/section-registry.ts\ndocSections.academy] B --> D[app/academy/layout.tsx\nSharedDocsLayout] D --> E[app/academy/slug/page.tsx\nDocsChromePage] D --> F[app/academy/slug/not-found.tsx\nAcademyNotFound] B --> G[lib/nav-links.tsx\nresourcesLinks Academy] H[content/academy/*.mdx] --> B H --> I[index.mdx] H --> J[ai-engineering-loop.mdx\n⚠️ broken MDX comment] H --> K[tracing.mdx\n⚠️ TODOs + incomplete text] H --> L[monitoring.mdx\n⚠️ TODOs + broken link] H --> M[datasets.mdx\n⚠️ TODOs] H --> N[experiments.mdx\n⚠️ TODOs] H --> O[evaluate.mdx\n⚠️ TODOs]Comments Outside Diff (2)
content/academy/ai-engineering-loop.mdx, line 102 (link)Multiple
TODO:andtodo:comments are embedded as plain text in all six content MDX files and will be displayed to visitors once these routes go live. The full routing infrastructure is wired up in this PR (source.config.ts,lib/source.ts,lib/section-registry.ts,app/academy/), so the pages will be publicly accessible. Examples on this page alone: lines 102, 112, 118, 130, 136, 144, 154, 162. The same pattern appears throughoutdatasets.mdx,evaluate.mdx,experiments.mdx,monitoring.mdx, andtracing.mdx.Prompt To Fix With AI
content/academy/monitoring.mdx, line 568 (link)The "What comes next" section links to
/academy/error-analysis, but that page is not part of this PR and does not exist in the repository. The TODO on line 572 even notes: "error analysis page does not exist in academy yet." Users clicking this link will hit the 404 not-found page.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "notes" | Re-trigger Greptile