Repository navigation
feat(docs): add PackageManagerTabs and register it globally - #433
Conversation
WalkthroughThe PR replaces post-build LLM tab flattening with a ChangesPackage manager tabs
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DocumentationPage
participant MDXComponents
participant PackageManagerTabs
participant Translator
DocumentationPage->>MDXComponents: Render PackageManagerTabs
MDXComponents->>PackageManagerTabs: Pass MDX children
PackageManagerTabs->>Translator: Validate and translate pnpm command
Translator-->>PackageManagerTabs: Return manager-specific commands
PackageManagerTabs-->>DocumentationPage: Render synchronized tabs
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preview DeploymentPreview URL: https://08eac494.bestax.pages.dev |
There was a problem hiding this comment.
Pull request overview
Adds a reusable Docusaurus MDX component to render package-manager-specific install commands (pnpm/nvm/yarn/bun), registers it globally for use across docs without per-page imports, and converts one existing docs snippet to validate the end-to-end pipeline.
Changes:
- Introduces
PackageManagerTabs(Tabs/TabItem + CodeBlock) that derives npm/yarn/bun commands from the sharedtranslate.mjsvocabulary. - Registers
PackageManagerTabsglobally viasrc/theme/MDXComponents.js. - Converts
docs/docs/guides/intro.md“Install Dependencies” to use<PackageManagerTabs ... />.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/src/theme/MDXComponents.js | Registers PackageManagerTabs globally in MDX so docs pages can use it without imports. |
| docs/src/components/PackageManagerTabs/index.js | New component rendering Docusaurus tabs + code blocks for each package manager. |
| docs/src/components/PackageManagerTabs/styles.module.css | Styles for tighter spacing and monospace tab labels. |
| docs/docs/guides/intro.md | Replaces a pnpm-only code fence with <PackageManagerTabs command="..." />. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export default function PackageManagerTabs({ command }) { | ||
| if (process.env.NODE_ENV === 'development') { | ||
| for (const warning of lintCommand(command)) { | ||
| console.warn(`PackageManagerTabs: ${warning}`); | ||
| } | ||
| } |
| <div className={styles.tabs}> | ||
| <Tabs groupId="package-manager" defaultValue={PACKAGE_MANAGERS[0]}> | ||
| {PACKAGE_MANAGERS.map(manager => ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 2 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | command="" (empty) renders four empty code blocks and passes the flatten gate as an empty ```bash fence — no dev warning, silent |
docs/src/components/PackageManagerTabs/index.js:26 |
| 2 | 🔵 Advisory | Robustness | The panel-margin reset keys off Docusaurus's internal hashed class [class*='tabItem_']; a theme upgrade could silently no-op it (cosmetic only) |
docs/src/components/PackageManagerTabs/styles.module.css:18 |
Overall: The change is sound and well-defended. The load-bearing invariant — "the pnpm tab a reader sees equals what an agent copies out of llms.txt" — holds by construction, because both the component and flatten-llms-tabs.mjs call the same renderCommand(cmd, 'pnpm') from the shared translate.mjs, and I confirmed the one converted fence (add @allxsmith/bestax-bulma) round-trips byte-identically to the original. Two independent CI safety nets back it: a missing global registration throws during the docusaurus build SSR prerender (naming the file), and any unflattened <PackageManagerTabs> JSX fails verifyArtifact — and I verified pnpm run build (turbo → docs build → the flattener) runs in CI at .github/workflows/ci.yml:52, so both nets fire on PRs. The JSX-without-React-import and the safe @theme-original/MDXComponents spread both match existing repo precedent (HomepageFeatures, the swizzled CodeBlock). The riskiest surface is the shared groupId="package-manager" contract the next PR's hero switcher must honor, but that's out of scope here. Human can merge; nothing needs action first.
Residual risk: the addressed failure class is drift between the rendered pnpm tab and the LLM artifacts.
- Missing / misspelled
commandprop — refuted: the flattener regex requires a literalcommand="...", so<PackageManagerTabs />or a typo'd attr leaves raw JSX that fails the gate and reddens CI. - Whitespace / verb-swap drift — refuted:
renderCommandis imported, not reimplemented, in the flattener; pnpm is a purepnpm ${segment}prefix over the same normalized segment, so component and artifact cannot diverge. - Empty-string
command— not fully refuted (advisory #1): it renders and flattens to an empty fence with no warning — an unlikely authoring slip, no drift, cosmetic only.
🏄 Clean little set wave, dude — one boring fence, but the pnpm-in-equals-pnpm-out current runs true end to end and the CI reef will chew up anything that drifts. Paddle it out, it's good to go.
The component half of #402. Renders one install command for pnpm, npm, yarn and bun, consuming the translate.mjs vocabulary added alongside the flattener. Registered in a new src/theme/MDXComponents.js rather than imported per page. ~20 pages will use it and none of the 135 .md files carries an import today, so per-file imports would put churn in exactly the diffs a reviewer needs to read closely — the ones nested in numbered steps. A missing global registration is still a loud failure (MDX throws during SSR prerender, naming the file), so this costs no safety. Tabs share groupId="package-manager", which is the reason to use @theme/Tabs rather than hand-rolling: Docusaurus persists the choice, so picking npm on one page selects npm everywhere, and the homepage hero switcher can later read the same slot. Converts one column-0 fence (guides/intro.md "Install Dependencies") to prove the pipeline end to end rather than landing the component unused. Verified in the browser: four tabs render with pnpm active by default, each tab shows the right translation, the selection persists across a reload, no console errors or hydration warnings, the swizzled CodeBlock's copy button still appears, and at 375px in dark mode all four labels fit on one line without overflowing. Verified in the artifacts: the flattened guides/intro.md twin reproduces the original fence exactly, so the LLM output is byte-identical to a pre-conversion baseline build. The conversion is invisible to agents, which is the invariant every later batch will be checked against.
Review follow-ups on #433. An omitted or empty `command` now throws instead of rendering. Deliberately not gated on NODE_ENV: `docusaurus build` prerenders every page with NODE_ENV=production, so an unconditional throw is what turns an authoring slip into a failed build naming the page — a dev-only check would miss it in CI. The flatten gate already catches a *missing* attribute, since its regex requires a literal command="…"; it cannot see command="" or command=" ; ", which flatten to a silent empty bash fence. Because the prerender covers every page, this can never fire in a browser on a site that built successfully. The default tab is now a named DEFAULT_PACKAGE_MANAGER rather than PACKAGE_MANAGERS[0], so tab order and the default can move independently. It is not merely cosmetic: the flattener collapses every tab group to the pnpm rendering, so the default is what makes the page agree with the artifact. TAB_GROUP_ID and TAB_STORAGE_KEY move into translate.mjs. The group id and the localStorage key it derives were about to be hardcoded in two places — the tab group here and the homepage hero switcher in #434 — where a rename would silently degrade to "hero and docs no longer share a choice" with nothing failing. Also documents the authoring convention in docs/CLAUDE.md, which described the component but never said how to write one, and records that .md renders JSX here because markdown.format defaults to mdx. Claude-Session: https://claude.ai/code/session_01TGA6sFTUGsJ6oXhfpjKEnh
d34d2dd to
5b0d3e6
Compare
Rebased onto main, review fixes applied — and one blocker foundRebased cleanly onto Review fixes —
|
Preview DeploymentPreview URL: https://7e0b1ed4.bestax.pages.dev |
…ener
The plugin that generates llms.txt strips PascalCase JSX tags and keeps their
inner text. Content in a *prop* is not inner text — it goes with the tag — so a
self-closing <PackageManagerTabs command="…" /> left an empty section in every
generated artifact while the rendered site looked fine.
Rather than post-process the artifacts, put the command where the rule already
preserves it. A page now wraps the pnpm fence it would have written anyway:
<PackageManagerTabs>
```bash
pnpm add @allxsmith/bestax-bulma
```
</PackageManagerTabs>
The plugin removes the wrapper and leaves exactly that fence, so the artifact is
correct by construction — no build step, no config, nothing to keep in sync.
Confirmed by running the plugin's own cleanMarkdownContent over both shapes: the
prop form yields an empty section, the children form yields the fence.
The fence is the single source of truth. The component recovers the authoring
vocabulary from it by stripping the `pnpm ` prefix line-wise, derives npm, yarn
and bun from that, and asserts the round trip renders back to the fence exactly
— so a non-canonical fence fails the prerender instead of producing three tabs
derived from something the page never showed.
That makes scripts/flatten-llms-tabs.mjs dead: its PackageManagerTabs path is
now redundant with the plugin, and its <Tabs> path can no longer work either,
since the labels it linearizes are props the plugin removes before the script
runs. Deleted with its four test suites, including the indentation suite whose
entire reason for existing was re-indenting a fence the script used to
synthesize. The two flattener assertions in the translate suite are replaced by
the identity the design now rests on: the pnpm rendering round-trips back to the
authored command.
Known and accepted: <TabItem label="…"> labels still vanish from the artifacts,
because label is Docusaurus's own prop and cannot move to children. Documented
in docs/CLAUDE.md and filed upstream as rachfop/docusaurus-plugin-llms#64.
Claude-Session: https://claude.ai/code/session_01TGA6sFTUGsJ6oXhfpjKEnh
Preview DeploymentPreview URL: https://c618e5c6.bestax.pages.dev |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/src/components/PackageManagerTabs/index.js:88
PackageManagerTabscan accept literal fenced-markdown text (e.g. when the fence inside the JSX block isn’t parsed as a code block due to missing blank lines). In that casetextOf(children)may include lines likebash/, and the current “round trip” check still passes because those lines are treated as passthrough segments—resulting in rendered tabs that show fence markers inside the code block instead of failing the build.
export default function PackageManagerTabs({ children }) {
// The fence as authored, minus the trailing newline MDX leaves on a code block.
const authored = textOf(children).replace(/\s+$/, '');
// Throws rather than rendering something wrong, and deliberately not gated on
// NODE_ENV: `docusaurus build` prerenders every page with NODE_ENV=production,
// so an unconditional throw is what turns an authoring slip into a failed build
// that names the page. A dev-only check would miss it in CI entirely — and
// because the prerender covers every page, this can never fire in a browser on
// a site that built successfully.
if (!authored) {
throw new Error(
'PackageManagerTabs: expected a pnpm code fence as its children, e.g.\n' +
'<PackageManagerTabs>\n\n```bash\npnpm add @allxsmith/bestax-bulma\n```\n\n</PackageManagerTabs>'
);
}
|
@claude please re Review, lots have changed |
There was a problem hiding this comment.
Deep review — 0 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Correctness | install --frozen-lockfile translates to npm install --frozen-lockfile, which npm ignores (it does a normal, lockfile-mutating install); the intended npm equivalent is npm ci. Not reached by this PR's one conversion, but a landmine for the conversion PRs. |
docs/src/components/PackageManagerTabs/translate.mjs:105 |
| 2 | 🔵 Advisory | Robustness | Deleting the flattener means generic <Tabs>/<TabItem label="…"> no longer promote labels to headings in the LLM artifacts — the bodies concatenate with nothing marking which option is which (skills/theming.mdx, skills/custom-component.mdx). |
docs/CLAUDE.md:39 |
| 3 | 🔵 Advisory | Coverage | The component's own logic (textOf, the pnpm-prefix unrender, and both prerender throws) has no unit test — docs has no jest/RTL harness, so only translate.mjs is exercised by node --test. The prerender throw is the only safety net. |
docs/src/components/PackageManagerTabs/index.js:64 |
Overall: The change is sound and the central design claim holds up under scrutiny. I confirmed empirically against the installed docusaurus-plugin-llms@0.5.1 that cleanMarkdownContent masks code fences before it strips PascalCase tags (content.js:204 then :221), so a fence wrapped in <PackageManagerTabs> survives verbatim while the wrapper is removed — the artifact really is correct by construction, and the deleted flattener/gate is genuinely redundant for this shape. The round-trip throw in index.js is a solid replacement safety net for the one component it guards. The riskiest part is not this PR but what it sets up: the translation table (item 1) and the label-loss regression (item 2) become user-visible only as later PRs convert real pages. A human should focus on item 1 before the contributing.md/security.md conversions land.
Residual risk: the failure class here is "the machine-readable artifact silently diverges from the rendered page."
- Fence dropped from the artifact — refuted: verified the plugin keeps masked code and strips only the wrapper tag; the per-page twin and
llms-full.txtboth reduce to the pnpm fence. - A bad/non-canonical fence renders 3 wrong tabs — refuted: the unconditional round-trip throw at
index.js:102fails the production prerender (which covers every page) before the artifact is written. - Wrong translated command reaches a reader — open (item 1):
pnpm install --frozen-lockfileexists today atcontributing.md:47, andsecurity.md:40itself states the npm equivalent isnpm ci— so when that page is converted, the npm tab will contradict the library's own guidance. No page emits it yet, hence advisory not blocking.
🏄 Clean set, dude — the fence rides straight through the plugin with the wrapper wiped, no wipeout. Just watch that
--frozen-lockfilereef when you paddle out to the next conversion; it's calm water today but it'll bite npm surfers later. Good to go.
|
Claude finished @allxsmith's task in 3m 56s —— View job Re-reviewing PR #433
What changed since the last reviewCommit <PackageManagerTabs>
```bash
pnpm add @allxsmith/bestax-bulmaThis is the right fix for the blocker the author called out on 2026-08-01 (the Also confirmed already fixed from the prior review round:
Finding: the round-trip guard has a real gap (the "suppressed" Copilot comment is correct)The 20:06 Copilot review flagged this as a suppressed/low-confidence comment on
That guarantee doesn't hold for exactly the failure mode the same paragraph warns about. If an author drops the blank line around the fence, MDX doesn't parse it as a code block, so |
…ences Review follow-ups on #433. **`install --frozen-lockfile` was wrong for npm and yarn.** npm has no such flag — `npm ci` *is* the frozen install — so `npm install --frozen-lockfile` did a normal, lockfile-mutating install: the opposite of what the reader asked for, and silent. guides/security.md already tells readers the npm equivalent is `npm ci`, so converting that page would have made it contradict itself. Yarn now gets Berry's `--immutable`, matching the `dlx` line that already targets Berry over Classic. bun takes the flag as written. No page emits this yet; it would have surfaced on the first conversion PR. **A leaked fence delimiter now throws.** If MDX doesn't parse the block — almost always the missing blank lines around it — the ``` delimiters arrive as text. The round-trip assertion cannot catch that: ```bash is not a known verb, so it passes through untouched on every tab and the equality still holds, while the tabs render fence markers inside the code block. Checked explicitly, with a message naming the actual mistake. There is a test asserting the round trip provably does *not* catch this, so the guard can't be removed as redundant. **The pnpm inverse moves into translate.mjs as `unrenderPnpm`.** It was inlined in a JSX file, where `node --test` cannot reach it — the round trip is the design's central invariant and its inverse had no direct coverage. Component and tests now share one definition. Not addressed, deliberately: <TabItem label> loss (already documented here and filed upstream), and full unit coverage of the component itself, which would need a jest/RTL harness `docs` does not have — the prerender throws run over every page on every build, which is why they are unconditional. Claude-Session: https://claude.ai/code/session_01TGA6sFTUGsJ6oXhfpjKEnh
Review round 2 — 2 fixed, 2 acknowledgedDeep review 1 —
|
| before | after | |
|---|---|---|
| pnpm | pnpm install --frozen-lockfile |
unchanged |
| npm | npm install --frozen-lockfile |
npm ci |
| yarn | yarn install --frozen-lockfile |
yarn install --immutable |
| bun | bun install --frozen-lockfile |
unchanged |
Plain install is unaffected. The table test covers all four.
Copilot (suppressed) — a leaked fence delimiter. Fixed, and it was real.
This one deserved not to be suppressed. If MDX doesn't parse the block — the missing blank
lines, which is the likely authoring slip and something docs/CLAUDE.md now warns about —
the ``` delimiters arrive as text. I reproduced it:
authored : "```bash\npnpm add foo\n```"
roundTrip : "```bash\npnpm add foo\n```" ← equal, so the assertion passes
npm tab : "```bash\nnpm install foo\n```"
The round trip provably cannot catch it: ```bash isn't a known verb, so it passes
through untouched on every tab and the equality still holds — while the tabs render fence
markers inside the code block. Now checked explicitly, with a message naming the real mistake.
There's a test asserting the round trip does not catch this, so the guard can't later be
deleted as redundant.
Deep review 3 — component coverage. Partly addressed.
The pnpm inverse moved into translate.mjs as unrenderPnpm and is now unit-tested; it was
inlined in a JSX file where node --test can't reach it, which was the weakest spot given the
round trip is the design's central invariant. Component and tests now share one definition.
textOf and the throws still have no direct unit test — that needs a jest/RTL harness docs
doesn't have, and adding one is out of scope here. Mitigation worth stating: the throws are
unconditional precisely so the production prerender exercises them across all 144 pages on
every build, so a broken textOf fails the build rather than shipping.
Deep review 2 — <Tabs> label loss. Acknowledged, no change.
Known and deliberate, documented in docs/CLAUDE.md with the two affected pages named, and
filed upstream as rachfop/docusaurus-plugin-llms#64.
21 tests pass, build clean, lint and format clean.
Preview DeploymentPreview URL: https://0ac2f6b9.bestax.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/package.json`:
- Line 9: Update the docs build script while preserving a generic tab-label
transformation for standard Docusaurus tabs before generating published LLM
artifacts. Do not rely solely on PackageManagerTabs; retain or replace the
functionality previously provided by flatten-llms-tabs.mjs so affected Markdown
includes each tab’s option label.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fa89b537-fa07-47b0-9d7e-3dbb88d1db39
📒 Files selected for processing (13)
docs/CLAUDE.mddocs/docs/guides/intro.mddocs/package.jsondocs/scripts/flatten-llms-corpus.test.mjsdocs/scripts/flatten-llms-gate.test.mjsdocs/scripts/flatten-llms-tabs.indent.test.mjsdocs/scripts/flatten-llms-tabs.mjsdocs/scripts/flatten-llms-tabs.test.mjsdocs/scripts/package-manager-translate.test.mjsdocs/src/components/PackageManagerTabs/index.jsdocs/src/components/PackageManagerTabs/styles.module.cssdocs/src/components/PackageManagerTabs/translate.mjsdocs/src/theme/MDXComponents.js
💤 Files with no reviewable changes (5)
- docs/scripts/flatten-llms-corpus.test.mjs
- docs/scripts/flatten-llms-tabs.test.mjs
- docs/scripts/flatten-llms-tabs.indent.test.mjs
- docs/scripts/flatten-llms-tabs.mjs
- docs/scripts/flatten-llms-gate.test.mjs
| "docs": "docusaurus start", | ||
| "start": "docusaurus start", | ||
| "build": "docusaurus build && node scripts/flatten-llms-tabs.mjs && node scripts/strip-generated-markers.mjs", | ||
| "build": "docusaurus build && node scripts/strip-generated-markers.mjs", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve labels for standard tabs in LLM artifacts.
Line 9 removes the only tab-flattening stage. docs/CLAUDE.md states that generated Markdown loses <TabItem> labels and identifies affected pages. The published LLM artifacts will contain tab bodies without their option names.
Keep a generic tab-label preservation step, or replace it before removing flatten-llms-tabs.mjs. PackageManagerTabs children solve this component’s command output only.
As per coding guidelines, “Keep documentation and the published LLM index accurate when documentation changes affect them.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/package.json` at line 9, Update the docs build script while preserving a
generic tab-label transformation for standard Docusaurus tabs before generating
published LLM artifacts. Do not rely solely on PackageManagerTabs; retain or
replace the functionality previously provided by flatten-llms-tabs.mjs so
affected Markdown includes each tab’s option label.
Source: Coding guidelines
|
🎉 This PR is included in version 4.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.8.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Second PR of #402. #418 (the flattener groundwork) has merged, so this targets
maindirectly.The component half. One install command, four package managers, pnpm first and default.
What's here
docs/src/components/PackageManagerTabs/index.js— consumes thetranslate.mjsvocabulary that landed with the flattener in chore(docs): make the LLM flattener indentation-aware before the #402 rollout #418, so the pnpm tab andllms.txtcannot drift.docs/src/theme/MDXComponents.js— global registration.guides/intro.md"Install Dependencies", to prove the pipeline rather than landing the component unused.Two decisions worth reviewing
Global registration over per-file imports. ~20 pages will use this and none of the 135
.mdfiles carries an import line today. Per-file imports would put churn in exactly the diffs a reviewer needs to read closely — the ones nested inside numbered steps. Forgetting a global registration is still a loud failure (MDX throws "Expected componentPackageManagerTabsto be defined" during the SSR prerender, naming the file), so this costs no safety. The tradeoff accepted: reading raw source on GitHub shows an element with no visible definition.Shared
groupId="package-manager". This is the reason to use@theme/Tabsrather than hand-rolling a switcher: Docusaurus persists the choice in localStorage, so picking npm on one page selects npm on every other page. The homepage hero switcher will read the same slot in the next PR. Note the tabvalues must stay exactly thePACKAGE_MANAGERSstrings — Docusaurus silently discards a stored value that isn't valid for the group.Verified in the browser
Dev server,
/docs/guides/intro:pnpmactive by default.npmswitches the panel and writesdocusaurus.tab.package-manager; the selection survives a reload.CodeBlock's copy button still appears inside the tab panel (non-liveblocks pass through untouched, as expected).Verified in the artifacts
The flattened
build/docs/guides/intro.mdtwin reproduces the original fence exactly:So
diff -rover*.mdtwins andllms*.txtagainst a pre-conversion baseline build is empty. The conversion is invisible to agents — that's the invariant every later batch gets checked against, and the reason it's worth establishing on one boring fence first.verifyArtifactreports clean onllms.txt,llms-full.txtand the converted twin. 70 tests pass,check:conformance8/8, prettier clean.Next
PR 2 is the homepage hero switcher; PRs 3-6 convert the docs pages, starting with the two riskiest shapes (the 2-space bulleted case and the fake-numbered-list case) before the 18-fence
react-setups.md.Summary by CodeRabbit
New Features
Documentation
Refactor