altdoc-multiversion-docs: redirect retired version directories to their replacements - #303
Conversation
A multiversion docs site publishes each version under its own subdirectory, so changing the naming scheme breaks every link written against the old one. The concrete case: a site that published the default branch's docs to /<branch>/ -- the insightsengineering/r-pkgdown-multiversion layout -- now publishes them to /dev/, this workflow's layout and pkgdown's own development-mode convention, leaving every /main/... link dead. Keeping /dev/ is right: it is what tidyverse and r-lib sites use, it matches the "(dev)" label the version dropdown already emits, and it survives a default-branch rename. So the fix belongs on the redirect side. The new `legacy-paths` input takes `old=new` pairs and generates a site-root 404.html that rewrites requests under old/ to the same path under new/. GitHub Pages serves no server-side redirects but does serve a site-root 404.html for unresolved paths, which is what makes deep links work -- a per-directory index.html meta-refresh only catches the directory root, and the broken links that prompted this are deep ones. Writing it into the same output directory as the root landing page also means it survives deployment: the "Deploy root landing page" step runs with clean: true and a clean-exclude list that does not name retired version directories. The pairs are explicit rather than derived from whichever version the build deployed, so /main/ resolves to the same place after a release build as after a default-branch build. Invalid pairs -- self-redirects, duplicates, malformed entries -- fail the step rather than publishing a dead or looping redirect. Test coverage runs at three levels in the altdoc-docs selftest job: pytest over the parsing and validation, a real `uses:` call to the composite, and a node harness that executes the generated page's own redirect script against a table of request paths. That last one exists because the Python tests can only assert the mapping reaches the page as text; whether a request then lands in the right place is a separate question, and getting it wrong is silent. Its table includes the paths that must not redirect, since a genuinely missing page under /dev/ would otherwise bounce forever. Closes #301 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UfkoVvS98hgAeW45cBkPm5
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code reviewReviewed the diff, cross-checked prose claims (GitHub Pages 404.html behavior, pkgdown dev-mode /dev/ convention, and insightsengineering/r-pkgdown-multiversion's per-branch layout -- all confirmed against their respective docs/source), traced the Python parsing/validation logic and the embedded JS redirect script against every selftest case plus edge cases (trailing slashes, deep links, prefix-collisions like /maintenance/ vs. main, query/fragment preservation, outside-base-path), and verified the three required doc-sync sites (README.md, website/workflows.qmd, website/reference/altdoc-multiversion-docs.qmd) plus the changelog fragment naming. One finding, posted inline: generate_legacy_redirects.py's main() duplicates generate_multiversion_landing_page.py's OUTPUT_DIR/mkdir/DOCS_BASE_URL-resolution boilerplate (including an identical comment) almost verbatim -- a DRY hit under CLAUDE.md's Code review guidelines section 2, consistent with this repo's own precedent of extracting comparably small duplicated snippets (extract-total-cost, sum-costs, build-reviewer-args) into shared helpers. No bugs found in the new redirect logic, the composite/workflow YAML wiring, or the two new test suites (pytest + the node harness that executes the real generated script). No AI-authorship prose tells, forward references, or fact-check issues found in the new prose beyond the one DRY finding above. VerdictNeeds minor work -- the DRY duplication is a small, non-blocking cleanup; everything else (correctness, tests, doc sync, changelog) checks out. |
|
Claude finished review — View run The top-level summary comment posted successfully: #303 (comment) Review summary for #303I reviewed the "redirect retired version directories" PR (adds Confirmed and posted inline (#303 (comment)):
Investigated but not flagged (false positives / non-issues, excluded from the final report):
No bugs found in the redirect logic, YAML wiring, or the two new test suites (pytest + node harness executing the real generated script). Fact-checked GitHub Pages' VerdictNeeds minor work — one small, non-blocking DRY cleanup; everything else (correctness, tests, doc sync, changelog) checks out. |
…enerators generate_legacy_redirects.py's main() repeated generate_multiversion_landing_page.py's OUTPUT_DIR/mkdir/DOCS_BASE_URL boilerplate, comment included. Both scripts live in the same composite action and run back to back, so the shared bits now live in _site_output.py and both import them -- the pattern this repo already uses for extract-total-cost, sum-costs, and build-reviewer-args, at a smaller scale. The part of that duplication that could actually bite was the "site-root" default, which appeared in both scripts AND in action.yml's own output-dir default -- three hand-maintained copies that had to agree. It is now one constant, and a test asserts it still matches action.yml rather than a comment asking the next reader to keep them in sync. The pytest module loads the script by file path, which (unlike the action's own `python3 <dir>/<script>.py` invocation) does not put the action directory on sys.path, so the test module inserts it explicitly before loading. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UfkoVvS98hgAeW45cBkPm5
2026-07-25 03:45 UTC |
Code reviewRe-reviewed after the latest commit (2592935, "Address review: share the output-dir/base-url plumbing between both generators"). Prior finding resolved. The previous review flagged that I re-verified the rest of the diff as well (redirect JS logic in One very minor, non-blocking note: the PR description's test-coverage table still says VerdictReady for merge — the outstanding DRY finding from the prior review is resolved, and no new issues found. |
|
Claude finished review — View run Posted the confirming review comment: #303 (comment) Summary: The latest commit (2592935) resolved the prior review's only finding — the DRY duplication between VerdictReady for merge |
Closes #301
The problem
altdoc-multiversion-docs.ymlpublishes the default branch's docs to/dev/.The scheme it replaces ---
insightsengineering/r-pkgdown-multiversion,which
UCD-SERG/serocalculatorused before its altdoc migration --- publishedto
gh-pages/<branch-or-tag>/, so the same docs lived at/main/.On serocalculator's
gh-pagestoday,main/holds two leftover font-awesomeCSS files and no
index.html, sohttps://ucd-serg.github.io/serocalculator/main/404s. Our own tracker has already rotted: UCD-SERG/serocalculator#575's body
links to
.../serocalculator/main/reference/index.html.Keeping
/dev/is the right call --- it is what tidyverse and r-lib sites use,it agrees with the
(dev)labelgenerate_version_dropdown.pyalready emits,and it survives a default-branch rename --- so the fix belongs on the redirect
side.
What this adds
A
legacy-pathsinput onaltdoc-multiversion-docs.yml, forwarded togenerate-altdoc-landing-page:That generates a site-root
404.htmlrewriting any request under/main/tothe same path under
/dev/. Empty (the default) generates nothing, so existingcallers are unaffected.
Why a 404 page, not a
main/index.htmlmeta-refreshsite-root
404.htmlfor unresolved paths, so one file covers/main/<anything>. A per-directory meta-refresh only catches the directoryroot --- and the links that prompted this are deep ones.
clean: trueand aclean-excludelist that does not name retired versiondirectories, so anything written outside
site-rootis deleted on the nextdefault-branch push.
Why explicit
old=newpairsThe landing target alternates between
devandlatest-tagdepending on whichevent ran. Deriving the redirect destination from it would make
/main/resolvesomewhere different after a release build than after a default-branch build.
Explicit pairs keep it stable.
Invalid pairs --- self-redirects, duplicates, malformed entries --- fail the
step rather than publishing a dead or looping redirect.
Test coverage
Three levels, all in the
altdoc-docsselftest job:tests/test_legacy_redirects.py(pytest, 30 cases)old=newparsing, each fail-fast validation, and the shared_site_outputplumbinguses: ./.github/actions/generate-altdoc-landing-pagecall withlegacy-pathssetgithub.action_pathresolution, the same proofrun-review-guard's e2e steps givetests/run-redirect-js-tests.mjs(node, 10 cases)The node harness exists because the Python tests can only assert the mapping
reaches the page as text. Whether a given request then lands in the right place
is a separate question, and getting it wrong is silent --- a bad redirect still
renders a plausible not-found page. Its table covers deep links, query/fragment
preservation, and the paths that must not redirect: a genuinely missing page
under
/dev/would otherwise bounce forever, and a directory that merely startswith a mapped name (
/maintenance/) is not a match.The existing no-
legacy-pathsassertion now also checks that no404.htmlisproduced, so the default stays a true no-op.
Shared plumbing (review round 2)
Both generator scripts now import
_site_output.pyrather than repeating theOUTPUT_DIR/DOCS_BASE_URLboilerplate --- the pattern this repo already usesfor
extract-total-cost,sum-costs, andbuild-reviewer-args.The part of that duplication that could actually bite was the
site-rootdefault, which lived in three hand-maintained places (both scripts and
action.yml's ownoutput-dirdefault) with nothing checking they agreed. Itis now one constant, and
test_output_dir_default_matches_action_ymlreadsaction.ymland asserts they still match, instead of a comment asking the nextreader to keep them in sync.
Known limitations, documented on the reference page
not-found notice linking to the docs root.
not just the mapped prefixes.
Docs synced
README.md,website/workflows.qmd,website/reference/altdoc-multiversion-docs.qmd(inputs table, a new "Retired version directories" section, and a commented
usage line in the example),
examples/altdoc-multiversion-docs.yml, theworkflow's own header comment, and
CLAUDE.md's Layout and Tests sections.Rollout note
altdoc-multiversion-docs.ymlcalls the composite at@v2, solegacy-pathsonly takes effect for consumers once the major tag advances past this merge ---
the same bootstrapping gap
CLAUDE.mddocuments for other new capabilities.The selftest uses the local
./ref, so CI covers this PR's code.UCD-SERG/serocalculatorpicks it up only after migrating off its bespokedocs.yaml(UCD-SERG/serocalculator#600) and then uncommenting the input(UCD-SERG/serocalculator#599).