chore: migrate offlinedocs to Fumadocs (render parity, single version) [DOCS-593] - #27390
Conversation
cd80df4 to
b99faf5
Compare
5f382a4 to
0b4a83c
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 10 | Last posted: Round 10, 64 findings (1 P0, 3 P1, 9 P2, 19 P3, 24 Nit, 8 Note), APPROVE. Review Finding inventoryFinding inventory, PR #27390Findings
Law analysisRound 5 re-analysis
Round 1 analysis
Contested and acknowledgedCRF-9 (P3, deleted
|
| Reviewer | Focus |
|---|---|
| Bisky | tests |
| Chopper | ops/errors |
| Churn-guard | change verification |
| Ging | language modernization |
| Gon | naming |
| Hisoka | edge cases |
| Killua | perf |
| Kite | change integrity |
| Knov | contracts |
| Knuckle | SQL |
| Komugi | flake/determinism |
| Kurapika | security |
| Law | decomposition |
| Leorio | docs |
| Luffy | product |
| Mafu-san | process |
| Mafuuu | contracts |
| Melody | dispatch/pairing |
| Meruem | structural |
| Nami | frontend |
| Netero | mechanical checks |
| Pariston | premise testing |
| Pen-botter | product gaps |
| Razor | verification |
| Robin | duplication |
| Ryosuke | Go arch |
| Takumi | concurrency |
| Zoro | shape |
🤖 Managed by Coder Agents.
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review only. Everything below comes from a single pre-panel reviewer that scans for mechanical and structural defects. The full review panel has not looked at this PR yet; it will review once these are addressed. Treat this as the floor, not the ceiling.
The migration itself holds up under execution rather than reading. pnpm lint (sync + fumadocs-mdx + typegen + tsc) passes, pnpm export produces 463 pages and 466 static routes with out/index.html at the root, so the make build/coder_docs_$(VERSION).tgz contract is intact, and git status stays clean after a full export, so check_unstaged.sh holds. The rendered corpus was checked for pipeline leakage: no literal \{, [step], [!NOTE], or class="tabs" survives into any of the 466 HTML files. escapeValue in os-tab.tsx was verified byte-identical to the copy Fumadocs does not export, so that duplication is justified and the comment saying so is accurate. The dead-code sweep confirmed stubComponents is load-bearing (<children></children> appears in three docs) and lucide-react belongs in the manifest as a declared peer despite having no import.
Counts: 1 P0, 2 P1, 1 P2, 6 P3, 3 Nit, 2 Note.
The P0 blocks CI, not just review. pnpm install --frozen-lockfile in offlinedocs/ fails on this head, which means the offlinedocs job and the release target both fail before anything is built. Reproduced independently. The lockfile has a second problem the first error hides: its importer block pins prettier 3.9.4 while the only prettier package and snapshot entries in the same file are 3.9.6, so the pinned specifier has no resolution at all. That shape says the lockfile was hand-edited rather than regenerated. Regenerate it.
Two findings are about things this PR knows are wrong and chose to log instead of fail. pnpm sync prints unmapped .md links=1 and exits 0, and the dead target reaches the shipped HTML (CRF-6). The image path has the same silent-fallthrough shape and is not even counted. A pipeline that can detect a broken inter-doc link should break the build, not the reader.
On the two follow-ups the description lists as out of scope: neither has a linked ticket, and this review assumes no follow-up exists. The search index is 46 MB served uncompressed on the reader's first keystroke, inside a 571 MB bundle where docs/images is already 93 MB. That is a human decision about airgapped download size, not one an agent on either side of this review can accept as permanent. File a ticket or state the acceptance explicitly (CRF-10).
The Storybook job is red on this head and could not be attributed to this PR from inside the review environment (gh has no credentials there). What was ruled out: the root lockfile has a single . importer and CI=1 pnpm install --frozen-lockfile at the root passes on this head, so the offlinedocs lockfile drift does not explain it. Someone with log access should confirm before it is treated as background noise (CRF-14).
My favorite line from the reviewer, on remark-code-meta.ts: "dead on arrival: no code fence anywhere in docs/ carries a meta string, so this plugin can never fire."
🤖 This review was automatically generated with Coder Agents.
babe71e to
9e44855
Compare
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
|
CI note (for @nickvigilante): the That is the fumadocs-mdx generated |
nickvigilante
changed the title
|
CI note (author FYI): the |
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2, still first-pass only. This is the second round scanned by the mechanical reviewer rather than the full panel; the panel reviews next round. Treat what follows as the floor again.
Every round-1 finding got a response, which is unusual and worth saying plainly. 12 fixed in code, 2 acknowledged with reasoning, 1 deferred with a ticket (DOCS-639), none silent, none contested without engagement. The fixes were verified rather than taken on trust: pnpm lint and pnpm test both exit 0 in a fresh worktree at this head, pnpm sync reports 463 pages, 51 meta.json, and 0 unmapped links, and the recursive install that broke Storybook in round 1 is green.
Two responses deserve specific credit. On CRF-14, you pulled the log and it corrected the review: round 1 concluded "the root install is not recursive" from a passing root pnpm install --frozen-lockfile and a single-importer root lockfile. Both observations were true and the conclusion was wrong, because pnpm/action-setup with run_install: true runs pnpm recursive install, which is a different command from the one that was tested. That is the review's error, not yours. On CRF-3, the fix was not a token test file: 15 tests asserting concrete input-to-output pairs that would fail against identity transforms, wired into a new CI step at .github/workflows/ci.yaml:1144.
Three items this round: 1 P2, 1 re-raise of the unfixed half of CRF-6, 1 Nit, 1 Note.
The P2 is the same class as CRF-3, one directory over. scripts/lib/transform.mjs now has tests; src/mdx-plugins/*.ts still has none, and it is 587 lines of equally pure tree surgery. Worth noting because the decomposition analysis on this PR already flagged "the plugin suite lands with no tests" as the condition that would make a mandatory split proposal, so this is the one open lever on that.
On CRF-6 you asked whether to close the image half now. Yes, close it. This review assumes no follow-up exists, so "cannot trigger today" is the whole safety argument and it expires the first time someone renames an image. The .md half hard-fails the release; the image half ships a broken link silently. The asymmetry is the finding.
A reviewer line I enjoyed, on why rehype-step-toc-numbers.ts deserves tests: it describes the code as a "numeric-vs-string dance across the data-fd-step/dataFdStep key rename by rehype-raw."
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review Round-3 re-request. All round-2 threads are resolved. Changes since round 2 (commit 1874ea8, CI green):
|
There was a problem hiding this comment.
Review is blocked this round on one unaddressed finding, and the reason it went unaddressed is this review's tooling, not anything you did.
The unaddressed item is CRF-18 (Note), on offlinedocs/src/mdx-plugins/remark-details-accordion.ts. It was raised in round 2 as a reply on the CRF-15 thread, and the bot posting these reviews lacks the GitHub permission to unresolve a thread (unresolveReviewThread: Resource not accessible by integration). So the finding has read as Resolved and Outdated in the UI since the moment it was written, and the round-2 review body named only CRF-6. Your round-3 summary enumerates CRF-16, CRF-17, and CRF-6, which is exactly what a finding nobody could see looks like. Restating it here, where nothing can collapse it.
CRF-18, in full: CRF-15 was fixed at the corpus by hoisting the <details> block out of the list item in docs/ai-coder/ai-gateway/monitoring.md, rather than by teaching the plugin to descend into list items. Reasonable call for a render-parity PR. The residual is that <details> nested inside a list item still falls back to a native disclosure, nothing in the plugin says so, and the corpus edit that dodges it carries no explanation either. The next docs author who nests a <details> under a bullet gets a silently different rendering and no signal. One comment on the sibling-scanning loop naming the list-item case closes it; teaching the plugin to descend is more than this PR needs to carry. If you disagree that it is worth a comment, say so on the thread and it closes as contested. A one-line "not worth it" with a reason is a complete response here.
What needs to happen for the panel to run: address CRF-18, or reply explaining why it should not be fixed, or file a ticket. Any of the three unblocks it. Nothing else is outstanding.
The rest of the round is clean. CRF-16 is closed properly: unit tests for all four plugins plus the shared mdast helper, colocated, 41 tests, and tsx over Node's built-in runner instead of pulling in a second framework, which is the right instinct. CRF-6's image half is closed symmetrically with the .md guard, and you verified it by pointing a doc at a missing image and confirming the non-zero exit and the named source -> target. CRF-17 is gone. Test density on the PR is now 13.7%, from 0.0% two rounds ago.
Two credits on CRF-8. You corrected your own round-1 reply, which is rarer than it should be: the four no-op overrides were already removed in 26323fe, so "kept as a deliberate mirror" was wrong and the end state was already in place. And you pushed back on "the root install is not recursive" with evidence rather than assertion, tracing run_install: true to [{recursive: true}] in the pinned action's dist/index.js and then demonstrating the split: the non-recursive root install passes against a drifted offlinedocs lockfile, pnpm -r install --frozen-lockfile fails with Scope: all 4 projects and ERR_PNPM_OUTDATED_LOCKFILE. That was the review's error and your correction is now recorded on it.
One process note while the panel waits. The panel has still not reviewed this PR. Three rounds have been first-pass and churn checks, so nothing here should be read as approval of the design, only as the mechanical floor being clean.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review Round-4 re-request. CRF-18 was the only open item, addressed in b99be00. I documented the limitation with a comment in All review threads are resolved. |
There was a problem hiding this comment.
First panel review. Rounds 1 through 3 were mechanical first-pass and churn rounds, so this is the first time the design has been looked at. 19 reviewers, 22 findings: 3 P2, 8 P3, 8 Nit, 3 Note. No P0 or P1.
The migration holds up under scrutiny, and the parts that were rebuilt in response to earlier rounds hold up best. Four reviewers returned nothing at all: determinism, TypeScript modernization, React modernization, and security. That last one matters given CRF-9; a security reviewer read the raw-HTML pipeline, the path resolution in the sync, and the asset copying, and did not file. The paired-chain reviewer walked five producer-to-consumer chains end to end (plugin order against rehype-raw's passThrough list, the [step] marker from normalizeStepHeadings through remarkSteps through the TOC shim, escapeValue byte-identity across the Fumadocs boundary, manifest to route to meta.json, and the pnpm override mirror) and found each one consistent. The sync's failure output drew unprompted praise from two reviewers for naming every source -> target pair and printing a per-class fix hint before exiting non-zero; that is now the template to copy for future sync failures.
The three P2s share a shape worth naming up front: each is a silent failure with a green build.
CRF-21 is the one I would fix first. Two docs whose basenames slugify to the same segment overwrite each other in the synced output, and the summary line counts writes issued rather than files that survived. Reproduced independently: a three-route manifest containing foo-bar.md and foo_bar.md emits two files, logs pages=3, and one page's content is simply gone. A.md and a.md, or x y.md and x-y.md, do the same. The corpus is clean today and the trigger is one filename in any future PR. This sync now hard-fails on a single unresolved link; it should not stay silent about losing a whole page.
CRF-20 is a class, not an instance. transform.mjs carries two scanners, fenceScan and the blockquote-aware mapProseLines, and only half the transforms use the aware one. Three consequences were reproduced separately by two reviewers and re-verified here: a comment straddling a fence deletes the fence opener and leaves the rest of the document inside a phantom fence; an HTML comment inside a blockquoted fence is stripped as prose; and a .md link inside a blockquoted code sample gets rewritten to a route, so a reader copying the snippet gets the post-rewrite path. The invariant the module wants is that fenced content is opaque to prose transforms, and nothing enforces it.
CRF-19 is about the one line that closed a P1. The values.includes(next) clamp is the entire fix for CRF-2, it has no test, offlinedocs has no component-test harness at all, and three separate couplings to unversioned fumadocs-ui internals sit underneath it. The plugin suite went from zero tests to 41 across two rounds; the component that actually owns the runtime coupling to the framework has none.
On severity spread, since several findings drew different ratings from different reviewers: CRF-19 and CRF-20 went up from P3 to P2 on the combined consequence, CRF-26 and CRF-29 went up from Note to P3, and CRF-21 was held at P2 rather than raised. Reasoning is in each comment. Nothing was dropped or downgraded.
One process note. The "Out of scope" bullet for the heading-anchor bug says "tracked separately" without naming the ticket, while the comparable bundle-size deferral names DOCS-639. Name it, so the claim is checkable.
A reviewer line that sums up CRF-25 better than I can: "The test appraises the wrong stone: it checks that expansion happened, not that expansion is safe."
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review Merged the latest Round-4 findings CRF-19 through CRF-40 were addressed in |
There was a problem hiding this comment.
Round 5. Requesting changes on one P1 that every prior round of this review missed.
First, the fixes. The round-4 response was structural rather than patch-level and it holds up under independent checks. transform.mjs now has one fence primitive and every prose transform routes through it, which five reviewers traced separately and agreed closes the class. routes.mjs and routes.test.mjs were extracted with coverage for collisions, file-vs-directory routes, manifest reorder, and duplicate entries. The jsdom harness is real: one reviewer removed the values.includes(next) clamp and confirmed the test fails, then restored it, so the PR description's claim about that test is verifiable and true. Another rendered seven before-and-after pairs through the actual unified pipeline to confirm the reworded .mdx-forward comments are accurate, including two real corpus edge cases. Six reviewers filed nothing at all, security and determinism among them.
Now the P1, CRF-42. extractTitle scans for the first H1 outside code fences and never accounts for YAML frontmatter. Every file produced by make gen opens with a frontmatter block whose second line is # Code generated by make gen. DO NOT EDIT., which matches the H1 regex. Reproduced on a fresh pnpm sync at this head: 195 of the 463 emitted pages carry title: "Code generated by make gen. DO NOT EDIT.". That is the entire REST API and CLI reference, roughly 42 percent of the corpus, and Fumadocs uses that frontmatter title for the sidebar entry, the page title, and the header. The sidebar under "REST API" and "Command Line" is 195 identical entries. Second consequence from the same root: h1Line splices out only the comment line, so the original --- delimiters survive into the emitted body and render as a horizontal rule followed by literal title: General text above the real content. The old renderer parsed frontmatter and preferred attributes.title, so this is a parity regression against the renderer being replaced.
I want to be straight about how this got here, because it reflects on the review more than on the PR. Five rounds, a 19-reviewer panel, and dozens of verified findings, and nobody read the emitted content/docs/** for the largest generated subtree in the corpus. Every pass checked transforms against synthetic inputs or against the code. The one reviewer who opened the output found it in the first place they looked.
CRF-43 is the other one worth attention. The round-4 refactor established mapOutsideInlineCode as the helper that keeps prose transforms off non-prose regions, and three transforms use it while two do not. Those two are rewriteContent and stripHtmlComments, so a .md link inside inline code gets rewritten to a route and an HTML comment inside inline code gets deleted. Both reproduced. This is the same asymmetry CRF-20 was rated P2 for, one level down, created by CRF-20's own fix, which is why it carries the same severity rather than the P3 both reviewers proposed.
CRF-35 is closed, and the record needed correcting. Your defense was that OSTab is emitted into content/docs/** and is therefore an authored-content contract. It is not: grep -rlF OSTab content/docs/ returns nothing, the generated files contain raw <div class="tabs">, and OSTab is created at MDX compile time by the remark plugin and consumed by mdx.tsx. Four reviewers checked and agreed, and I reproduced it. The disposition survives anyway, because at Nit severity with no runtime consequence a standalone rename is a fair call. Two panel reviewers repeated your premise as fact without testing it, which is a failure on our side, not yours.
Second ask on a description item: the "Out of scope" bullet still says the heading-anchor bug is "tracked separately" without naming the ticket, while the neighboring deferral names DOCS-639. This was raised in the round-4 body and is unchanged. Name it or drop the bullet.
Decomposition was re-analyzed this round because effective additions grew from 2105 to 3803. The verdict is unchanged and advisory: split vertically next time, do not block this. The reasoning is empirical rather than procedural, that each concern did in fact get per-concern scrutiny across five rounds. That analysis also retracted its own round-1 claim that remarkSteps must travel with the step transforms.
From the reviewer who found the inline-code gap, on how it eventually surfaces: "Would you like to see it fire on a page that ships? Ship one that talks about how links work."
🤖 This review was automatically generated with Coder Agents.
041d632 to
6de7807
Compare
|
/coder-agents-review Superseding my previous re-request: I replaced the earlier merge-commit resolution with a clean rebase onto the latest
|
There was a problem hiding this comment.
No review this round. The offlinedocs tree is unchanged since round 5, so there is nothing new to look at, and all nine open findings are still open.
git diff --stat 041d6326b..6de7807c2 -- offlinedocs/ is empty. The 12 changed files are docs/** prose arriving from main. That matches your own description of the rebase: the offlinedocs tree is byte-identical to the previously reviewed resolution. The rebase itself is fine and the linear history is an improvement; it just does not move any finding.
On timing, because it likely explains this. The round-5 review was submitted at 17:41:47Z and your re-request at 17:56:44Z, fifteen minutes later, and it talks only about replacing the merge commit with a rebase. My read is that the re-request was already in flight and you have not seen the round-5 review yet, rather than that these were considered and set aside. If that is right, this comment is just a pointer to it.
Still open, in priority order:
CRF-42 (P1), offlinedocs/scripts/lib/transform.mjs:384. extractTitle has no notion of YAML frontmatter, so the # Code generated by make gen. DO NOT EDIT. line inside the frontmatter block matches its H1 scan. 195 of the 463 emitted pages, the whole REST API and CLI reference, ship with that string as their Fumadocs title, which drives the sidebar entry, the page title, and the header. Same root cause leaves the original --- delimiters in the body, so those pages render a horizontal rule and a literal title: General paragraph above the content. Reproduced on a fresh pnpm sync at this head. The deleted renderer read attributes.title from frontmatter, so this is a parity regression against the thing being replaced. This one is why the review is blocked.
CRF-43 (P2), offlinedocs/scripts/lib/transform.mjs:456. rewriteContent and stripHtmlComments do not route through mapOutsideInlineCode, so a .md link inside inline code is rewritten to a route and an HTML comment inside inline code is deleted. Both reproduced.
CRF-44 (P3), offlinedocs/scripts/sync-docs.mjs:290. A manifest route with no backing file is dropped from the sidebar with no error, unlike the four defect classes the sync does fail on.
CRF-45 (P3), offlinedocs/src/components/os-tab.tsx:53. detectOS and its three UA regexes have no coverage; both jsdom tests preseed storage and skip that branch.
CRF-41 (Nit), offlinedocs/src/components/mdx.tsx:89. {...props} spreads after target and rel, so caller attributes win over the safe defaults.
CRF-46 (Nit), offlinedocs/scripts/sync-docs.mjs:147. Module-scope currentSourceRel duplicates rewriteContent's rel argument.
CRF-47 (Nit), offlinedocs/src/mdx-plugins/rehype-step-toc-numbers.test.ts:7. HastNode is still duplicated between plugin and test.
CRF-48 (Nit), offlinedocs/scripts/lib/transform.mjs:121. quoted_delim breaks the file's camelCase convention.
CRF-49 (Note), docs/about/contributing/documentation.md:36. No change requested; it records a pattern to watch and one reviewer supplied counter-evidence in your favor on both instances.
Each one needs a fix, a reply explaining why it should not be fixed, or a linked ticket. Any of the three unblocks the round. Nothing else is outstanding, and the round-4 work verified in round 5 still stands: the fence-tracker refactor, the routes extraction, and the jsdom harness were all checked independently and hold.
One detail worth flagging separately: your validation list for this head reports pnpm export at 466 pages and pnpm lint reporting 0 unmapped links and 0 unresolved images. Both are true and neither would catch CRF-42. The sync's summary line counts what it wrote, not whether what it wrote is right, which is exactly how a wrong title on 42 percent of the corpus passed five rounds of green checks.
🤖 This review was automatically generated with Coder Agents.
|
Re-review requested. This round addresses all 9 open findings from the round-5 review, in
Each inline thread carries the specifics and the resolving commit. Green on /coder-agents-review |
There was a problem hiding this comment.
The P1 is fixed and I verified it rather than taking the claim. Dismissing the round-5 changes-requested. One new P2, which this round introduced while fixing the P1.
CRF-42 verification: a fresh pnpm sync at this head emits 463 pages, and I scanned every one of them for a missing, empty, overlong, or DO NOT EDIT title and for a stray --- block at the top of the body. Zero real anomalies. The single hit is tutorials/best-practices/organizations.md, where the source genuinely has a thematic break under its H1, which is CRF-51 below. reference/api/general.md is title: "General", reference/cli/ssh.md is title: "ssh" with its description intact. Four panel reviewers re-read the emitted corpus separately and agree. The fix is at the root: parseFrontmatter is consulted first and the precedence chain is frontmatter title, body H1, manifest label, segment.
The rest of the round-5 work holds up too. createRewriteCtx deletes the module-scope variable rather than documenting it. findUnbackedManifestRoutes uses the same collect-then-name-then-fail shape as the four sibling defect classes instead of inventing a fifth. detectOS(items, ua) is now pure with the UA injected. Nine of nineteen reviewers filed nothing, and one ran the jsdom suite 20 times serially and 15 times in parallel looking for flake and found none.
Now the P2, CRF-52, and it is the round-5 lesson repeating one column over. This round started preferring the source's frontmatter description over the manifest, which routes three CLI pages through the new parser for the first time. That parser strips the outer quotes with value.slice(1, -1) and decodes nothing, so a YAML \" survives as a literal backslash-quote all the way into the rendered description, the <meta name="description">, and the search index. Two reviewers found it independently with byte-level evidence and I reproduced it. My own CRF-42 verification checked the title column of the emitted frontmatter and did not look at the description column immediately beside it, which is the same mistake in miniature that let the P1 live for four rounds.
The recommended fix comes from a third reviewer and is worth more than the bug report: fumadocs-core is already a direct dependency here and publicly exports content/md/frontmatter, which is js-yaml backed. It was checked against the same four corner cases your hand-rolled parser handles, including the make-gen comment block and the nested state: list. Adopting it closes CRF-52 and CRF-50 together and deletes about fifty lines, including the loose regex behind CRF-50.
On the config docstring commit that landed alongside the fixes: it is inert and cleanly disclosed, and I downgraded it from three P2s to one Nit. Worth saying plainly though, three reviewers independently flagged it as scope drift in a commit whose subject is a P1 fix. Keeping prose sweeps out of fix commits makes both easier to review.
Two description items with no code line to attach them to. The validation list says pnpm test is 59 tests; the suite is 66, verified at this head. And the "Out of scope" heading-anchor bullet still says "tracked separately" without naming a ticket, which has now gone unaddressed across rounds 4, 5, and 7 while the neighbouring deferral names DOCS-639. Name it or drop it.
From the reviewer who found the description corruption, on where this review keeps looking: "The CRF-42 verification landed on the title column of that output; the description column one over shipped corrupted anyway."
🤖 This review was automatically generated with Coder Agents.
0701ec0 to
b8dc545
Compare
|
/coder-agents-review |
- CRF-58: skip CommonMark angle-bracket link destinations ([text](<url>)) in the autolink rewrite so a URL containing parentheses is not corrupted into a nested [url](url). Fixes a 404 on the homepage self-hosted link. - CRF-59: wrap the js-yaml frontmatter parse error as a labeled FrontmatterError and collect-and-name it in sync-docs, so a corpus file with invalid YAML frontmatter fails the sync naming the file instead of crashing with an opaque parser stack trace. - CRF-61: drop redundant reactStrictMode: true (App Router default). - CRF-62: reword a stale test comment describing the removed hand-rolled frontmatter parser. CRF-60 (GitHub-alert case sensitivity) is deferred: it involves docs-corpus edits and a render-parity question for Nick.
…regex The GitHub-alert MARKER regex matches only the canonical uppercase, no-space form. Add a comment recording that this is intentional, so the matcher stays aligned with GitHub's canonical syntax and non-canonical corpus markers are normalized at the source instead of accommodated in this pipeline. Surfaced by CRF-60 in the offlinedocs Fumadocs migration review.
…omments CRF-63: add assertions that a plain parenthetical autolink (<url>) is still rewritten (the admin setup Maven link depends on it) and that the bracketed-email lookbehind holds, pinning the two CRF-58 branches the prior test left uncovered. CRF-64: trim the try/catch comment that restated the FrontmatterError class doc, and tighten the MARKER strictness comment to lead with the rationale.
fumadocs-mdx intermittently writes an empty .source/server.ts when it regenerates over an existing .source (in CI, postinstall generates a stub, then lint regenerates after sync), which fails tsc with a spurious TS2306 'is not a module'. This is the flaky offlinedocs Lint failure seen across review rounds. Add scripts/ensure-source.mjs after fumadocs-mdx in lint:types: it verifies the generated entry files are non-empty and regenerates up to 3 times if not, so tsc never runs against a half-written .source. It is a no-op in the normal case (a couple of stat calls). Verified locally: forcing server.ts empty triggers one regenerate and recovers; the happy path is untouched.
b5c6650 to
0b48ffd
Compare
The empty .source/server.ts that intermittently broke tsc (TS2306) was written by next typegen, which ran after the guard. Move node scripts/ensure-source.mjs to run after next typegen and immediately before tsc, so it validates the final generated state and regenerates via fumadocs-mdx when a step leaves server.ts empty.
monitoring.md is corpus content, not migration code, so its callout/details edits belong on main rather than in this PR. Reset the file to main so the migration diff stays offlinedocs-only. This backs out the CRF-15 corpus lift; the AI Gateway possible-client-values note now renders from main's native nested <details>, which still builds and renders as a working disclosure in the static export (verified), so nothing about offlinedocs breaks. A styled accordion for that note can be handled in the renderer as a follow-up if desired.
Back out the straight-vertical-rail TOC flattening that hid Fumadocs' depth-stepped SVG rail and repainted a ::before rail while keeping the step-number badges; that combination misaligned the badges in UAT (bpmct review on PR #27390). Restoring the Fumadocs default rail lines the badges up again. Christin's separate rule that hides the trailing Next up / Next steps TOC entry is kept. The flattened-rail design is deferred for discussion in DOCS-686.
…otcha Expand the output:export comment to name the reported symptom (the UI renders but nothing is clickable) and its cause (a stale service worker at the dev origin, and next dev erroring on the non-prerendered /serviceWorker.js under output:export), plus the recovery (unregister the service worker; the dev server runs on :26337). Gives anyone running the offline docs locally, human or agent, what they need to avoid the dead-page trap.
|
re: supergraphic images for offlinedocs eg
I already added minified .webp light/dark supergraphics to the site directory there; maybe those should be reused? |
@untra I'm inclined to agree, and I'm getting the exact statistics on package size of current vs. new pre-webp vs new with webp. |
This doesn't reproduce for me. Can you check again?
As discussed in our 1:1, I believe this is a problem with the dev server. From Coder Agents: Cause: output: "export" was set for every build, including next dev. In export mode Next installs a catch-all route that rejects any path generateStaticParams didn't prerender. A browser that had previously loaded the old offlinedocs still requests /serviceWorker.js (a leftover service-worker registration), plus favicon probes, etc. Those are non-prerendered paths, so next dev errored on them instead of serving normally, and the local site came up dead/non-interactive. Your boss hit it because he spun the site up on a machine/browser that had the old site's service worker registered at localhost:3000. Already fixed in the PR by two commits:
Same cause as the OS tabs issue
Same cause as the OS tabs issue
I backed out this part of the change, and we'll discuss this with Christin and Tracy separately, but this should now be fixed, and the "worm" is back. |
Add regression coverage for the docs homepage frontmatter shape (docs/README.md: a title-less mapping with only markdown_url). parseFrontmatter must treat it as frontmatter and extractTitle must strip the whole block (frontmatterEnd covers it), with the title falling back to the body H1, so markdown_url never renders as plain text in the page. Reported in UAT as duplicate frontmatter rendering as text on the homepage; the current sync already strips it (verified in generated content and the static build), and this guards against regression.
Convert the offlinedocs supergraphic backgrounds from PNG to webp (q80): screenshot-bg-light 442KB -> 44KB and screenshot-bg-dark 298KB -> 48KB (~722KB -> ~89KB total, ~88% smaller), and point the brand.css background-image rules at the .webp files. The light background loads on every page, so this is a real per-page load win. Offline-package impact is negligible (~0.5% of the ~141MB bundle). Raised by Sam in UAT. DOCS-688.
|
@untra I swapped out the existing supergraphics to use your webp supergraphics, and it didn't move the needle much in terms of size, but they're there. The thing that occupies most of the increase in storage space of the docs package is the ~45 MB search index, since I want users to be able to search through the docs. The size increase is a trade-off I'm willing to make here. Let me know if you have other feedback on the rest of the offline docs. |
… [DOCS-661] (#28180) ## What Two small docs fixes, split out of the offlinedocs Fumadocs migration (#27390) so that PR stays a focused tooling change and these land on their own review track. - `docs/about/contributing/documentation.md`: the Vale style-guide link pointed at `docs/.style/style-guide/`, which is excluded from the published docs corpus, so the relative link 404s on the live site. Repointed to an absolute github.com URL. - `docs/ai-coder/ai-gateway/monitoring.md`: lifted the "Possible client values" `<details>` block out of the list item so it renders as a top-level accordion, and restored a dropped blockquote continuation marker (`>`). ## Why Both are genuine docs bugs on their own: - The `.style/` link is broken on the live site today, because that directory is intentionally excluded from the published corpus. - The `<details>` nested inside a list item renders inconsistently; lifting it to a sibling block is portable across renderers. They are also prerequisites for the offlinedocs Fumadocs migration, whose stricter build-time sync hard-fails on unmapped inter-doc links and only converts a `<details>` to an accordion when it is a list sibling. Landing them here first lets the migration PR drop these two files on its next rebase. ## Validation - `make fmt/markdown` and `make lint/markdown`: clean (via pre-commit-light). - Docs-only change; no CI or build config touched. > This PR was created with AI assistance (Coder Agents).
Fumadocs' ScrollArea renders its own Radix scrollbar (client-side, fades when idle). The sidebar CSS re-enables the native scrollbar for an always-visible affordance, so once the nav overflows both painted at once. Hide the Radix scrollbar in the sidebar so only the native one remains.
Rewrite the loop-and-mutate bodies of buildDirRoutes, the buildFileMap collision scan, findUnbackedManifestRoutes, minOrderUnder, and the buildMeta item construction as map/filter/reduce, dropping the local let/for/push scaffolding. Behavior is unchanged and the route/transform unit tests still pass. Higher-risk loops (manifest walk, dir model, line scanners, sync-docs I/O) are left for a follow-up.
Move the architectural narrative from routes.mjs header and section comments into scripts/lib/README.md, leaving a one-line pointer at each function. Addresses review feedback on comment density while preserving the specific rationale earlier review rounds asked to document.
Move the transform.mjs header essays (fence-scanner design, the .mdx-forward escapes, and the frontmatter/title and link-rewrite contracts) into scripts/lib/README.md, leaving terse function summaries and the short at-code notes. Same comment-density compromise already applied to routes.mjs.
| function walk(nodes) { | ||
| for (const node of nodes || []) { | ||
| const r = manifestRoute(node, dirRoutes); | ||
| if (r !== null) { | ||
| if (!manifestMeta.has(r)) { | ||
| manifestMeta.set(r, { | ||
| title: node.title, | ||
| description: node.description, | ||
| }); | ||
| manifestPathByRoute.set(r, node.path); | ||
| } | ||
| if (!routeOrder.has(r)) routeOrder.set(r, order++); | ||
| } | ||
| if (node.children && node.children.length) { | ||
| const childRoutes = node.children | ||
| .map((child) => manifestRoute(child, dirRoutes)) | ||
| .filter((x) => x !== null); | ||
| if (r !== null && r !== "") childOrderByDir.set(r, childRoutes); | ||
| walk(node.children); | ||
| } | ||
| } | ||
| } | ||
| walk(manifest.routes); |
There was a problem hiding this comment.
it's a recursive function here. this smells of bug potential.
ask your llm to review this section of code , and ask it to guard against an infinite depth. How deep should a manifest tree before it's too far?
There was a problem hiding this comment.
Human me/TL,DR: you're right to take pause on recursive functions in general, but in this specific scenario, I'm confident that recursion is safe and will always terminate. The manifest it's talking about here is manifest.json, which will never have a cycle, even if a page is its own predecessor/ancestor. I've also used recursion in other SSGs (Jekyll, for example) to render sidebars.
I can add a unit test that feeds a deeply-nested manifest and asserts it still builds correctly. Let me know.
AI me: Thanks - I reviewed the manifest walk (buildManifestModel) specifically for this. Short version: recursion is safe here, and a depth guard would gate valid input against an arbitrary number rather than prevent a real failure.
Why it can't run away:
- The input is
docs/manifest.json, parsed withJSON.parse. JSON has no references, so it cannot encode a cycle - it is always a finite tree. There is no "infinite depth" state to reach; the recursion always terminates. - The base case is already there:
walkiteratesnodes || []and only recurses whennode.children?.lengthis truthy, so a childless node ends that branch. Depth equals the manifest's literal nesting depth and nothing else. - That depth is hand-authored nav nesting, a few levels (section -> subsection -> page). V8 handles on the order of 10k stack frames, so a stack overflow would need a manifest nested thousands of levels deep. This is a build-time script over a committed, trusted file, not runtime or untrusted input.
Why I did not add a maxDepth guard:
- It would guard a state that cannot occur, and it would introduce a failure mode we do not have today: a legitimately deep but valid manifest would start failing the build at whatever threshold we pick. "How deep is too far" has no principled answer, because there is no real limit to enforce - any number is arbitrary.
- If stack depth were the actual risk, the fix would be an explicit iterative stack, not a cap - and that reintroduces the
let/.push/mutation we just moved away from, to defend against input JSON cannot produce. - The sync already hard-fails on the manifest and corpus problems that can actually happen: output collisions, unmapped inter-doc links, unresolved images, and unclosed HTML comments. Runaway recursion is not in that set because it cannot occur.
Your instinct is the right one for untrusted or potentially cyclic input (a user-supplied graph, or a filesystem walk that can hit a symlink loop). It just does not transfer to a static acyclic JSON tree.
If it is useful for peace of mind, I am happy to add a unit test that feeds a deeply-nested manifest and asserts it still builds correctly - that documents "deep nesting is fine" without inventing a cap. Let me know.
This response was generated with Coder Agents.
There was a problem hiding this comment.
as long as it's finite, that's fine. but also know you can set a depth limit to enforce a design constraint on layout structure.
The generated root meta.json sorted the About and Support sections after Reference, so Reference was not the last top-level section. The manifest's top-level "About" section maps to the homepage (README, route ""), so the about/ and support/ directories it owns through nested children are not among the manifest's top-level routes and fell into the "ordered by own manifest position" bucket, which sorts after every listed section. Build the root order from the directories each top-level section introduces, expanding only sections that own no top-level directory of their own (a homepage-mapped or pure grouping section). about/ and support/ now land at the About section's position and Reference stays last, and sections that merely link into another section's directory no longer pull it out of place. Add routes.test.mjs coverage for the homepage-mapped case, manifest-vs-disk order, and cross-section links. DOCS-706
… [DOCS-661] (#28180) ## What Two small docs fixes, split out of the offlinedocs Fumadocs migration (#27390) so that PR stays a focused tooling change and these land on their own review track. - `docs/about/contributing/documentation.md`: the Vale style-guide link pointed at `docs/.style/style-guide/`, which is excluded from the published docs corpus, so the relative link 404s on the live site. Repointed to an absolute github.com URL. - `docs/ai-coder/ai-gateway/monitoring.md`: lifted the "Possible client values" `<details>` block out of the list item so it renders as a top-level accordion, and restored a dropped blockquote continuation marker (`>`). ## Why Both are genuine docs bugs on their own: - The `.style/` link is broken on the live site today, because that directory is intentionally excluded from the published corpus. - The `<details>` nested inside a list item renders inconsistently; lifting it to a sibling block is portable across renderers. They are also prerequisites for the offlinedocs Fumadocs migration, whose stricter build-time sync hard-fails on unmapped inter-doc links and only converts a `<details>` to an accordion when it is a list sibling. Landing them here first lets the migration PR drop these two files on its next rebase. ## Validation - `make fmt/markdown` and `make lint/markdown`: clean (via pre-commit-light). - Docs-only change; no CI or build config touched. > This PR was created with AI assistance (Coder Agents).
| const text = (value: string): MdastNode => ({ type: "text", value }); | ||
| const para = (children: MdastNode[]): MdastNode => ({ | ||
| type: "paragraph", | ||
| children, | ||
| }); | ||
| const quote = (children: MdastNode[]): MdastNode => ({ | ||
| type: "blockquote", | ||
| children, | ||
| }); | ||
| const kids = (n: MdastNode): MdastNode[] => (n.children ?? []) as MdastNode[]; | ||
| const attrs = (n: MdastNode): MdastNode[] => | ||
| (n.attributes as MdastNode[]) ?? []; | ||
|
|
||
| function run(children: MdastNode[]): MdastNode { | ||
| const tree: MdastNode = { type: "root", children }; | ||
| remarkGithubCallouts()(tree); | ||
| return tree; | ||
| } |
There was a problem hiding this comment.
not sure I love these little test helpers (especially kids() lol) since you need to grok those first before the tests make any sense. consider inlining these smaller ones at least
|
|
||
| // Every directory route implied by the corpus (for docs/a/b/c.md: "a" and | ||
| // "a/b"). See README.md ("Directory routes and index collisions"). | ||
| export function buildDirRoutes(allMd) { |
There was a problem hiding this comment.
coming from typescript, this whole file is kind of tough to read when you don't know what allMd is. could you add jsdoc annotations to this and the other mjs files?
| * These stub components are a SECONDARY guard: if any document is ever treated | ||
| * as MDX (renamed to `.mdx`, or a future config change), the unknown tags | ||
| * resolve to an inert passthrough instead of throwing | ||
| * "Expected component `X` to be defined". | ||
| */ | ||
| const stubComponents: MDXComponents = { | ||
| children: Passthrough, | ||
| Children: Passthrough, | ||
| }; |
There was a problem hiding this comment.
this seems a little too clever. is renaming a file to mdx a real concern? if so, would it be better to fail loudly with a more descriptive error message?
aslilac
left a comment
aslilac
left a comment
There was a problem hiding this comment.
this pr is entirely too large to be well reviewed
- could this be done more granularly? does it need to all be done in one fell swoop?
- could we start by introducing a partial "offlinedocs-next/" directory that can live independently while kinks get ironed out, then a separate pr to remove the existing next.js implementation
- could each plugin be its own pr discussing the design and what problem it is meant to solve?
a pr this large with this much going on cannot actually be understood. there's no way to tell what change has what effect without spending days on it. there's an overwhelming amount of information buried in here and it's unreasonable to expect it to be sifted through effectively. there have already been 177 comments on this pr, and that's not even including the ones I am about to leave from this review. most of them are essay length and written by robots. that is not the sign of a healthy contribution.
| // Rewrite GitHub-style alerts (`> [!NOTE]`) into `Callout`, | ||
| // `<div class="tabs">` blocks into `Tabs`/`Tab`, and `<details>`/`<summary>` | ||
| // blocks into `Accordions`. All run before the default plugins | ||
| // (structure/TOC) and the rehype stage so downstream sees the rewritten | ||
| // tree. Tabs runs before the accordion pass so a tabs block inside a | ||
| // `<details>` is converted first, then wrapped. | ||
| // `remarkSteps` (Fumadocs core) groups consecutive `## Title [step]` | ||
| // headings into a numbered `.fd-steps` structure. The `[step]` markers are | ||
| // baked into the synced source by `normalizeStepHeadings` (refer to | ||
| // scripts/lib/transform.mjs), so no custom step plugin runs here. | ||
| // Prepending is safe: these plugins match core Markdown blockquotes and raw | ||
| // HTML nodes, independent of any default plugin. |
There was a problem hiding this comment.
yap, and misplaced. this isn't one big comment, this is several comments tripping over each other. there's a comment in here specifically about the remarkGithubCallouts plugin, another about remarkCoderTabs, another about remarkDetailsAccordion, etc. this could easily be broken into five comments totaling in half as many words.
| // A `<details>` block is recognized only when its opener and closer arrive as | ||
| // sibling raw-HTML nodes: DETAILS_OPEN must start the opening node and | ||
| // DETAILS_CLOSE must end the closing node. `transform` recurses into every | ||
| // parent first, so a `<details>` nested inside a list item is still converted, | ||
| // as long as those two delimiters are clean. | ||
| // | ||
| // Known limitation: a closing node that carries trailing markup on the same | ||
| // line, e.g. `</details><br>`, does not match DETAILS_CLOSE, so the block is | ||
| // left as a native `<details>` disclosure instead of an Accordion. The fallback | ||
| // still renders (the inner Markdown, including callouts, converts); it is just | ||
| // not Accordion-styled. Author `<details>` and `</details>` each alone on their | ||
| // own line to get the Accordion. A `<details>` that contains another | ||
| // `<details>` also falls back to a native disclosure. Depth-balancing is not | ||
| // attempted. |
| // Convert raw `<details>`/`<summary>` HTML blocks into Fumadocs `Accordions`. | ||
| // | ||
| // In the plain-Markdown (.md) pipeline, `<details>`, `<summary>...</summary>`, | ||
| // and `</details>` arrive as separate raw `html` mdast nodes with the body | ||
| // content as normal Markdown nodes in between (the same shape the corpus uses | ||
| // for `<div class="tabs">`). This plugin finds that run, lifts the summary text | ||
| // into an accordion title, and wraps the body in a `Callout`-style | ||
| // `mdxJsxFlowElement` (`Accordions` > `Accordion`) so it renders through the | ||
| // component map. Runs at the mdast level, so no per-file rewrites are needed. | ||
| // | ||
| // Each `<details>` becomes its own single-item `<Accordions type="single">`, | ||
| // which Fumadocs renders collapsible: it opens and closes exactly like the | ||
| // native element. The summary is flattened to text (any inline tags such as | ||
| // `<code>` are dropped) because the title is passed as a string attribute. |
| // The coder/coder corpus authors tabs as a raw HTML wrapper whose immediate | ||
| // headings are the tab labels: | ||
| // | ||
| // <div class="tabs"> | ||
| // | ||
| // ### UI | ||
| // ...content... | ||
| // | ||
| // ### CLI | ||
| // ...content... | ||
| // | ||
| // </div> | ||
| // | ||
| // In the plain-Markdown (.md) pipeline those `<div ...>`/`</div>` lines arrive | ||
| // as raw `html` mdast nodes with the headings and body as normal Markdown nodes | ||
| // between them. This plugin finds each wrapper, splits the inner nodes on the | ||
| // first heading depth, and emits one tab per heading so it renders through the | ||
| // component map. | ||
| // | ||
| // On top of that base conversion it applies three authoring upgrades: | ||
| // | ||
| // * Nested tabs. Inner `<div class="tabs">` blocks are converted before the | ||
| // outer split, so a distro picker inside a "Linux" tab stays nested instead | ||
| // of being flattened into sibling tabs (e.g. install/uninstall). | ||
| // * Split bundled OS labels. A heading whose label is only operating-system | ||
| // names joined by `/`, `,`, `&`, or `and` (e.g. `Linux/macOS`) becomes one | ||
| // tab per OS, with the content duplicated, so OS detection can target a | ||
| // single operating system. | ||
| // * Grouping + OS awareness. A set whose labels are all operating systems is | ||
| // emitted as `OSTab` (shared `os` group + a user-agent default). Every | ||
| // other multi-tab set is emitted as `Tabs` with a `groupId` derived from | ||
| // its labels and `persist`, so repeated sets (CLI/UI, Docker/Kubernetes, | ||
| // ...) stay in sync and survive reloads. |
| // Restore numeric `data-fd-step` on step headings after `rehype-raw`. | ||
| // | ||
| // Fumadocs numbers steps in the table of contents through its default | ||
| // `rehypeToc` plugin, which copies a heading's step index onto the TOC entry | ||
| // only when the property is a real number: | ||
| // | ||
| // _step: typeof element.properties["data-fd-step"] === "number" ? ... : undefined | ||
| // | ||
| // `remarkSteps` sets `data-fd-step` as a number, but this site runs `rehype-raw` | ||
| // first (to keep the corpus's raw HTML). `rehype-raw` serializes and reparses | ||
| // the whole tree, which turns that number into a string and renames the hast | ||
| // key to camelCase (`dataFdStep`). By the time `rehypeToc` runs, the numeric | ||
| // check fails and the TOC loses its step numbers (the body circles are | ||
| // unaffected, since they come from the class-based `.fd-step` CSS counter). | ||
| // | ||
| // This plugin runs between `rehype-raw` and the default plugins and rewrites | ||
| // the value back to a number under the dashed key `rehypeToc` reads. |
| // Clamp the shared group value to an OS this set actually offers. | ||
| // Without this guard, a set that omits the stored OS (e.g. a | ||
| // macOS/Windows set when another set has stored `os = linux`) adopts | ||
| // `linux`, matches no panel, and renders an empty box - the exact | ||
| // regression that shipped once before this clamp existed. | ||
| // | ||
| // The clamp sits on three couplings to fumadocs-ui internals; a | ||
| // refactor that breaks any of them silently reintroduces the empty | ||
| // box: (1) this set's storage key (OS_GROUP) is the same key | ||
| // fumadocs' groupId store reads and writes; (2) escapeValue here | ||
| // matches fumadocs' own label escaping byte-for-byte, so `values` | ||
| // compares against the same strings; (3) fumadocs applies a stored | ||
| // group value by calling onValueChange (rather than writing its | ||
| // internal state directly), which is where this runs. |
| /** | ||
| * OS-aware Tabs. | ||
| * | ||
| * Renders Fumadocs Tabs under a shared `os` group so every OS tab set stays in | ||
| * sync, and, on the first visit with no stored choice, selects the tab matching | ||
| * the reader's operating system. | ||
| * | ||
| * Detection is client-only. Fumadocs' Tabs reads the group store in a mount | ||
| * layout effect, so seeding `sessionStorage` during the first client render | ||
| * (never on the server) sets the initial tab without changing the hydrated | ||
| * markup, so there is no hydration mismatch. A persisted explicit choice | ||
| * (`localStorage`, written by Tabs on click) is left untouched, and if the OS | ||
| * cannot be detected or is not one of the tabs, Tabs keeps its default. | ||
| * | ||
| * OS tab sets offer different subsets of operating systems (some show all | ||
| * three, some only macOS/Windows). The `os` group value is shared across every | ||
| * set, so a value one set stores (e.g. `linux`) is applied to every other set | ||
| * too. Fumadocs only guards that in its styled `Tabs` when `items` is passed, | ||
| * which also renders a plain text tab list and would drop the OS icons. So we | ||
| * build on the unstyled container with a controlled value and clamp updates to | ||
| * the operating systems this set actually offers: an out-of-set value is | ||
| * ignored (this set keeps its default) while the shared choice stays intact for | ||
| * the sets that can honor it. | ||
| */ |
| // The root's children are the top-level directories and files in the order | ||
| // their manifest section introduces them. A section is normally a top-level | ||
| // directory (get-started, reference), contributing that one segment. A | ||
| // section with no page of its own - one mapped to the homepage (About -> | ||
| // README, route "") or a pure grouping node - instead owns its directories | ||
| // through nested children (about/, support/), so those are collected from its | ||
| // subtree and land at the section's position. Only such sections expand, so a | ||
| // normal section that merely links into another's directory does not pull | ||
| // that segment out of its own place. |
| // jsdom environment for the offlinedocs component tests, loaded as a Node | ||
| // --import preload so the DOM globals exist before React DOM and fumadocs-ui | ||
| // are imported by any test file. Kept as plain .mjs outside src/ so it is not | ||
| // part of the TypeScript build and needs no jsdom type stubs. | ||
| // | ||
| // The component under test (OSTab) drives fumadocs-ui's Radix-based Tabs, which | ||
| // expect a handful of browser globals jsdom does not provide on its own | ||
| // (ResizeObserver, matchMedia, requestAnimationFrame, scrollIntoView). They are | ||
| // stubbed minimally: the tests assert tab-panel state, not layout or animation. |
| // Emit a fully static site into out/ so the docs bundle is self-contained | ||
| // and can be served by any static file host with no Node server. This is | ||
| // what the release pipeline tars into coder_docs_<version>.tgz for | ||
| // offline/airgapped use. | ||
| // | ||
| // Only export for production builds (`next build`). Under `output: export` | ||
| // Next installs an optional catch-all route that rejects any path | ||
| // generateStaticParams did not prerender. In dev that turns incidental | ||
| // requests into hard errors instead of clean 404s - most notably a stale | ||
| // /serviceWorker.js still registered at the dev origin by a prior site (a | ||
| // local coder server or the old offlinedocs, both of which used :3000), | ||
| // plus favicon probes and similar. When next dev errors on those, the page | ||
| // renders but never becomes interactive; the symptom reported in UAT was | ||
| // "nothing in the UI is clickable" (OS tabs, theme toggle, etc.). Gating | ||
| // export to production keeps next dev on Next's normal server (clean 404s), | ||
| // so local dev stays interactive, while next build still produces the full | ||
| // static export. | ||
| // | ||
| // Running the docs anywhere: if a machine shows a dead or unclickable page | ||
| // from an earlier session, a stale service worker is cached - unregister it | ||
| // (DevTools > Application > Service workers > Unregister) and hard-reload. | ||
| // The dev server also runs on :26337 (see the package.json scripts), not | ||
| // :3000, to stay off that shared origin. |
6 participants
What
Migrates
offlinedocs/from the Next.js pages-router +react-markdown+ Chakra renderer to a Fumadocs App Router static export. Phase 1 of moving the offline docs onto Fumadocs: coder/coder docs corpus only, single version, images bundled locally for offline/airgapped use.This is a renderer swap at content/render parity: every page renders (463 pages, filtered to
manifest.jsonroutes), now with Fumadocs' UI plus the Coder brand theme (and dark mode), rather than pixel-identical Chakra output.Why
src/mdx-plugins/and the sync transforms inscripts/lib/) on Fumadocs. offlinedocs is the only Fumadocs consumer in this repo today, so this does not share a pipeline with the hosted docs yet; the payoff is that if the hosted docs later migrate to Fumadocs, sharing this plugin surface becomes a directory move rather than a rewrite.How it works
scripts/sync-docs.mjs) reads this repo's owndocs/tree +manifest.jsonand emits a Fumadocs content source (content/docs/**, generated + gitignored) plus locally bundled images (public/images, gitignored). No network access.next buildwithoutput: exportwrites the static site toofflinedocs/out/, and the corpus is emitted as.md+rehype-rawso raw HTML (tables,<details>, etc.) renders as it does on GitHub.scripts/lib/transform.mjsand the pure route/ordering logic inscripts/lib/routes.mjs; the remark/rehype plugins live insrc/mdx-plugins/.Output contract preserved
offlinedocs/out/index.htmlat root;make build/coder_docs_<version>.tgzstill tarsofflinedocs/out/for the release pipeline (release.yaml/tag-and-release.yaml), unchanged.offlinedocsjob contract is unchanged:pnpm format:check,pnpm lint,make build/coder_docs_<version>.tgz,check_unstaged.sh.Notes for reviewers
offlinedocs/package.jsondeclarespnpm.overridesmirroring the repo-root overrides. This matters because two CI paths consumeofflinedocs/pnpm-lock.yaml: theofflinedocsjob's scopedcd offlinedocs && pnpm install(also used to build the release tarball), and theStorybookjob's recursive root install (pnpm/action-setupwithrun_install: true), which enumeratesofflinedocs,site, andscripts/apidocgenand validates each project's lockfile with--frozen-lockfile. That recursive pass, not the root's own single-importerpnpm install, is what a lockfile drift breaks. pnpm applies overrides from the workspace root, so offlinedocs' block is recorded into its own lockfile by the scoped install. Onlyjs-yamlandpicomatchresolve to packages in offlinedocs' tree, so those are the only two entries kept.jsdomis a new dev dependency (pinned to25.0.1for Node compatibility) backing anode:testcomponent-test harness forOSTab(test/jsdom-setup.mjspreload +src/components/os-tab.test.tsx). It renders the component against the real fumadocs-ui Tabs to pin the shared-OS-group clamp against regression. Dev-only; it is not shipped in the bundle.Validation (local)
pnpm format:checkandpnpm lint(sync + fumadocs-mdx + typegen + tsc): pass.pnpm test: 67 tests pass (31scripts/**transforms + routes, 31src/mdx-plugins/**plugins, 5src/components/**jsdom render tests).pnpm export(sync +next build,output: export): 463 pages (manifest routes only),index.htmlatout/root, sync reports 0 unmapped links and 0 unresolved images.<details>accordions, Shiki code highlighting, clean inter-doc links (no.md), source-tree links rewritten togithub.com, and bundled images.Review updates
Changes since the initial draft (some intentionally beyond render parity, per review):
manifest.jsonexactly: the generated tree is filtered to manifest routes, dropping non-manifest orphan files so the offline nav matches what coder.com publishes.ImageZoom).next devno longer errors on incidental non-prerendered requests (output: exportis applied to production builds only).@phosphor-icons/react, the icon set coder.com uses).next dev -p 26337) instead of 3000, which collided withcoder server's default access URL.staticGET, emitted toout/api/search) plus Fumadocs'staticclient, so the bundle is searchable entirely client-side with no server and no external network calls. 463 pages indexed; adds ~46 MB to the bundle.Round 4 review (coder-agents-review): findings addressed
Resolves the full-panel review findings CRF-19 through CRF-40 in commit
1687ad94e. Highlights:scripts/lib/routes.mjswith table tests; the generatedmeta.jsonis byte-identical to before the refactor across the 463-page corpus.<!--comment that would blank a page (both name the offending source).../../coderd, ...) are rewritten togithub.com/coder/coderURLs at sync time (112 links across 37 pages) instead of shipping dead relative links.<details>fallback,structuredCloneisolation test,MdastNodeconsolidation, tuple-basedmeta.jsonordering,.mdx-forward comment rewrites, and dead-code/boilerplate removal.Out of scope (follow-ups)
docs/imagestree, as before). Tracked in DOCS-639.Local testing
If the docs render but the UI is unclickable (OS tabs, theme toggle, sidebar), the cause is almost always a stale service worker cached in the browser at the dev origin. A prior site on
http://localhost:3000(a localcoder serveror the old offlinedocs) can leave a service worker registered there; it then intercepts requests for this app and serves stale assets, so the page never hydrates and nothing responds to clicks.This PR mitigates it two ways:
:26337instead of:3000, so it no longer shares an origin with a service worker registered by another local app.output: "export"is applied only to productionnext build, notnext dev. In export mode Next adds a catch-all route that rejects any non-prerendered path; a stale/serviceWorker.jsrequest is exactly such a path, sonext devused to error on it. Dev now uses Next's normal server (clean 404s).If a machine still shows a dead page from an earlier session, unregister the service worker (DevTools > Application > Service workers > Unregister) and hard-reload.
Security note: raw HTML rendering
Document bodies now render as raw HTML (
remarkRehypeOptions.allowDangerousHtml+rehype-rawinsource.config.ts); the previous renderer'ssanitize-htmlpass is intentionally dropped. Raw-HTML fidelity with the corpus (tables,<details>, and similar) is the goal of this change, and the input is coder/coder's own committeddocs/tree, which is authored and human-reviewed rather than untrusted user input. Nothing replaces the sanitizer today; if offlinedocs ever renders content from an untrusted source, reintroduce sanitization at that boundary. (coder-agents-review CRF-9.)Draft pending review. Linear: DOCS-593, DOCS-599.