Add per-tool documentation versioning (Latest/Stable/Previous versions) - #14
Conversation
Each Robotiq-maintained, submodule-synced tool (Tactile Sensor C++/Python, Adaptive grippers C++, Isaac Sim) now gets its own independent Latest/Stable/Previous-versions switcher, backed by its own Docusaurus plugin-content-docs instance with content under versioned-tools/ instead of docs/. A shared nav-tree module (scripts/site-nav-tree.mjs) keeps the full site sidebar visible on every versioned page instead of swapping in an isolated one, using plain encodeURI()-escaped links rather than Docusaurus's pathname:// scheme (which silently opens links in a new tab). The version dropdown is scoped to its own tool via a swizzled navbar item, and the default version banner is shortened to one line. Also fixes CI's "generated content is committed" check to cover versioned-tools/ (generate-tools-table.js rewrites wrapper pages there too) and adds the matching versioned-tools/**/*.md gitignore rule so synced content isn't accidentally tracked. docs/contribute/ is updated throughout to document the new architecture, including a new versioning.mdx walkthrough and a fix for a pre-existing stale reference to a deleted script. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mbegin-robotiq
left a comment
mbegin-robotiq
left a comment
There was a problem hiding this comment.
I reviewed the code changes, not the synced or generated doc content, and reproduced #3 and #4 with real npm run build runs. Each inline comment includes a suggested fix and the unit tests I'd add.
Test setup proposal: right now the repo only has the post-build smoke test (npm test runs scripts/check-build.js). All the tests proposed here can use Node's built-in node:test with no new dependency. For example, add "test:unit": "node --test test/" and a npm run test:unit step in ci.yml before the build, since none of them need a build or network access. Some logic needs to move out of the CLI entry points so it can be imported: parseLsRemote from listTags, and pruneStale plus the legacy cleanup from the top-level job loop in sync-external-docs.js.
Unrelated to this PR: every build, including a clean one, warns about a broken anchor. 04-robust-example-walkthrough.md:69 links to 01-environment-setup.md#serial-port-notes, but that heading is ## Serial port settings. The fix belongs in the robotiq/grippers repo.
mbegin-robotiq
left a comment
mbegin-robotiq
left a comment
There was a problem hiding this comment.
Approving so this can move forward, but the 4 inline comments from my previous review need to be resolved before merging (annotated-tag SHA, frozen site nav in versioned sidebars, YouTube embed breaking .mdx, stale synced files causing duplicate routes).
The plugin emitted a raw mdast 'html' node unconditionally. Docusaurus runs rehype-raw for plain Markdown but not for MDX, so an .mdx page using the thumbnail-link pattern would fail the build with "Cannot handle unknown node `raw`" — flagged in review on #11, never actually fixed before that PR was approved and merged. Now branches on the file's own extension (available on the vfile passed to the transformer): .md keeps emitting the raw html node, .mdx emits an equivalent mdxJsxFlowElement tree instead. Verified against a throwaway .md and .mdx page using the same pattern — both now render the identical <div class="video-wrapper"><iframe ...></iframe></div> output, where before the .mdx one failed to build at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses every inline comment from review, each reproduced before
fixing:
- scripts/list-submodule-tags.js: an annotated tag's SHA was the tag
object's own SHA, not the commit it points at (git ls-remote emits
both; the `^{}` peeled line was being dropped instead of preferred).
A Stable pin or Previous-versions link built from this pointed at
the wrong object. Parsing logic extracted into a pure `parseLsRemote`
for testing.
- src/remark/youtubeEmbed.mjs: emitted a raw mdast `html` node
unconditionally, which Docusaurus can't turn into HTML on an `.mdx`
page (no rehype-raw there) — would fail any `.mdx` page hitting the
thumbnail-link pattern with "Cannot handle unknown node `raw`". Now
emits an `mdxJsxFlowElement` unconditionally instead, which compiles
correctly for both `.md` (via rehype-raw's own passThrough list) and
`.mdx` — one node shape, no format branching needed, and no manual
HTML-escaping either.
- scripts/site-nav-tree.mjs: `buildInstanceSidebar`'s full output gets
frozen into every cut version's `*_versioned_sidebars/*.json`
snapshot, so a later change to the shared site nav tree (renamed
label, moved page, new product) never reached an already-cut
version. Added `extractActiveItem`/`regenerateInstanceSidebar` plus
scripts/regenerate-versioned-sidebars.mjs, wired into `npm run
generate`, so every snapshot is kept in sync automatically and
ci.yml's existing drift check now covers these files too.
- scripts/sync-external-docs.js: a job that gained `destRoot` (moved
its output from docs/ to versioned-tools/) left its OLD output under
docs/drivers/ untouched on a checkout that had already synced
against an older commit — reproduced as 49 duplicate-route warnings
on a real build. Added cleanupLegacyDestRoot, run for every destRoot
job (including the doxygen2docusaurus one), which prunes that legacy
location on every sync going forward.
Also adds a `node:test`-based unit test harness (`npm run test:unit`,
wired into ci.yml before Build) covering all four fixes plus a
drift-guard regression test for the versioned-sidebar staleness issue.
pruneStale/cleanupLegacyDestRoot moved into scripts/lib/prune.js so
they're importable/testable without running the full sync pipeline.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
All 4 inline findings fixed in 260a153, replied to each individually with what changed and how I verified it. Summary:
Test setup: adopted as proposed — Broken anchor — agreed this is out of scope here; the fix belongs in |
There was a problem hiding this comment.
Verdict: changes requested (posted as a comment). The versioning mechanism looks solid. The blocking items are inline: the site currently defaults to main, and it needs to steer users to released versions and mark main's API as experimental.
Out of scope for this PR (SDK repos): mark main-only APIs with \since vX.Y / \experimental Doxygen tags (or an experimental namespace), and keep a CHANGELOG "Unreleased" section.
Pulls in PR #11 (merged) and two Dependabot submodule bumps (2f85_cpp, isaacsim_assets) that landed on main since this branch diverged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> # Conflicts: # docs/contribute/how-it-works.mdx # src/remark/youtubeEmbed.mjs
Blocking items from review:
- The instance root URL (a bare tool link, search result, or first
visit) served Development (main) content, not a release — swapped
the per-tool `versions` config so Stable owns the root path ('') and
Development moves to '/next', banner-tagged 'unreleased' and marked
noIndex so it's excluded from search/sitemap. Renamed "Latest" to
"Development (main)" throughout (Docusaurus's own convention reserves
"latest" for the newest *release*).
- Required a follow-on fix: doxygen2docusaurus bakes every internal
cross-reference as an absolute URL using the plugin's bare
routeBasePath, with no idea Docusaurus versioning exists. Moving
Development off the instance root broke every one of those links
(every API page 404ing on itself) until `currentVersionPath` was
added to thread that tool's `versions.current.path` through to the
doxygen2docusaurus job.
- Added docs/api-stability.mdx (only a tagged release is a
compatibility commitment; main is experimental; the vMAJOR.MINOR.PATCH
scheme), linked from the version banner, docs/intro.mdx, and a new
"Use a released version" admonition on Development's own wrapper
pages. Every doxygen2docusaurus-generated API page also gets a short
experimental notice injected right after its frontmatter — "where
users copy code from" per the review.
- Rewrote the version banner to state the actual policy instead of just
"unreleased".
Other findings fixed:
- Stable snapshots' README/source links pointed at `/tree/main/` and
`/blob/main/` instead of the tag they're supposed to represent (the
file at main can already differ from what shipped). Rewrote all 3
found. Documented the gap: no automation for this yet, has to be
checked by hand on every cut (see versioning.mdx).
- Wrong img alt text ("2F-85 gripper" on the C++/Python logos, copy-
pasted boilerplate) across all 6 wrapper/snapshot pages, plus real
typos ("developped", "the the", "Are below are") in the Tactile
Sensor wrapper pages.
- Dropped draft/documentation-versioning.md and its summary (.md + .pdf)
— folded the still-relevant "why this design" rationale into
versioning.mdx instead of linking out to a design-exploration doc not
meant to live in this repo. Updated ~15 code-comment references
across the codebase to point at versioning.mdx instead.
- Reworded 3 passages in versioning.mdx that read as session narrative
("the user rejected it on sight", "this exact mistake has happened
twice already") into neutral rules with rationale.
Not done in this pass (flagged as improvements/discussion, not
blocking, and each is its own real scope):
- A `scripts/cut-version.js` to automate the manual checkout/sync/
version/restore sequence, and deriving the Stable label from
metadata written at cut time instead of hand-typing it. The manual
process is now documented accurately (including the main→tag
rewrite step this pass added), but still fragile — worth a follow-up.
- Whether Development's own API reference should be publicly indexed
at all vs. kept to a preview/internal deploy — noIndex now keeps it
out of search either way; left the publish-or-not question open per
the review's own "for discussion" framing.
Also merges main (PR #11, two Dependabot bumps) — this branch had
fallen behind.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Replied to each of the 12 inline comments individually (fixed in 7e3f1dc). Summary: Blocking, all fixed:
Also fixed:
Deferred, explained in the relevant thread each:
Merged |
test:unit failed on CI (ubuntu-latest, Node 20) with "Could not find
'.../test/**/*.test.js'" — node --test's own glob-matching for
positional args, which the local dev environment's Node 24 supports,
isn't available on Node 20. A bare directory argument (node --test
test/, closer to the reviewer's original suggestion) isn't safe either:
it reproducibly fails locally (Windows, Node 24) with an unrelated
"Cannot find module" error, untested on Linux/Node 20.
Replaced both with scripts/run-unit-tests.js: discovers test/*.test.{js,mjs}
via a plain fs.readdirSync and passes the file list to node:test's
programmatic run() API. No CLI glob or directory-argument ambiguity,
works identically regardless of OS or Node version.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Correction to the test-setup note in my earlier comment: the explicit glob strings ( |
3 participants
Summary
versioned-tools/instead ofdocs/. Stable owns the tool's root URL; Development lives at/next, banner-taggedunreleasedand excluded from search (noIndex).scripts/site-nav-tree.mjs), using plainencodeURI()-escaped links rather than Docusaurus'spathname://scheme, which silently opens links in a new tab instead of navigating. Every already-cut version's frozen sidebar snapshot is kept in sync with that tree automatically viascripts/regenerate-versioned-sidebars.mjs, wired intonpm run generate.docs/api-stability.mdx: only a tagged release (Stable) is a compatibility commitment; Development (main) is experimental. Linked from the version banner,docs/intro.mdx, a "Use a released version" admonition on every Development wrapper page, and a short notice injected into every generated API reference page.scripts/sync-external-docs.jscleans up a tool's pre-destRootlegacy output on every sync, so an existing local checkout doesn't end up serving duplicate routes.node:testunit harness (npm run test:unit, wired into CI before Build) covering the tag-parsing logic, the stale-file pruning logic, the YouTube-embed remark plugin (both.mdand.mdxcompilation), and a drift guard for the versioned-sidebar snapshots.docs/contribute/is updated throughout, including a newversioning.mdxwalkthrough of the whole mechanism, gotchas, and the manual version-cutting procedure.Follow-ups (not in this PR, flagged in review)
scripts/cut-version.jsto automate the manual checkout/sync/version/restore/restore cutting procedure (currently documented, but manual) — would also be the natural place to automate themain→tag source-link rewrite and derive each tool's Stable label from metadata instead of hand-typing it indocusaurus.config.js.noIndexcurrently covers the main practical risk either way) — open question, not resolved.Test plan
npm run build) succeeds with no new broken links/anchors, no duplicate routesnpm testandnpm run test:unit(33 tests) pass/nexttarget="_blank"(only genuine external links do)docs/intro.mdx's site-wide tables still list all relocated tools/next🤖 Generated with Claude Code