Repository navigation
Fix docs SSR retention during the VuePress upgrade - #6814
Conversation
Build on the dependency/config migration from espnet#6641. Skip server-side content-update callback registration with a versioned client patch, and document its removal after an upstream release.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: espnet/espnet/.coderabbit.yaml Review profile: QUIET Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe VuePress package updates its framework, bundler, theme, and search dependencies. Installation runs Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No concrete issue currently prevents merging. Complete the pending documentation build and rendered-site checks as normal validation. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6814 +/- ##
==========================================
- Coverage 73.34% 68.97% -4.37%
==========================================
Files 858 850 -8
Lines 80259 79603 -656
==========================================
- Hits 58866 54907 -3959
- Misses 21393 24696 +3303
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sw005320
left a comment
sw005320
left a comment
There was a problem hiding this comment.
Thank you, this is the fix - I verified it on the real corpus, see my comment on #6644: same generated doc/vuepress/src/ (1904 pages), same 4 GB heap, patch reversed dies at 3983 MB mid-render, patch applied renders in 77.75 s. The mechanism (SSR never unmounts, so contentUpdatedCallbacks retains every page) is also what vuepress/core#1722 describes.
One blocker and two small requests before merging.
Blocker: the lockfile is not in sync with package.json. With Node 22.22 / npm 10.9.7:
npm ci
npm error `npm ci` can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync.
npm error Missing: markdown-it@15.0.2 from lock file
npm error Missing: argparse@3.0.2 from lock file
npm error Missing: entities@8.1.0 from lock file
npm error Missing: linkify-it@6.1.0 from lock file
npm error Missing: uc.micro@3.0.0 from lock file
(repeated 10x)
The nested markdown-it@15 trees under @vuepress/plugin-markdown-{chart,hint,image,include,math,preview,stylize,tab}, @vuepress/plugin-icon and vuepress-plugin-md-enhance are missing. npm install regenerates them (+959 lines) and npm audit on the result is still 0 vulnerabilities. CI did not catch this because ci/doc.sh runs npm i, which repairs the lockfile silently, and CodeRabbit ignores package-lock.json. Please run npm install in doc/vuepress, confirm npm ci passes on the result, and commit the regenerated lockfile.
Please link the upstream PR. patches/README.md says the fix is not released upstream; vuepress/core#1722 is the upstream PR with the identical change. Referencing it makes it clear when this patch can be dropped.
Optional: theme.ts is now identical to fix/vuepress-rc31-upgrade minus the comments that explained the four rc.109 renames (plugins.mdEnhance -> markdown, codetabs -> codeTabs, iconAssets -> plugins.icon.assets, searchPro -> plugins.slimsearch, autoSuggestions -> suggestion). I would keep them; nothing else in the repo records why those keys moved.
Also, the doc job skipped on this run because the Documentation label was applied a few seconds after the pull_request event and the label check reads the frozen event payload. A rerun will not help; your next push will.
|
Thanks @sw005320. Addressed in e547048: regenerated the lockfile, linked vuepress/core#1722, and restored the config migration comments. CI now uses |
|
✅ Action performedFull review finished. |
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
|
Thanks, @meryemsakin! |
2 participants
What did you change?
Resume the documentation dependency upgrade from #6641: VuePress rc.31,
theme-hope rc.109, and the matching search/config migration.
Apply a small, versioned
patch-packagepatch to@vuepress/clientduringinstallation. It skips
onContentUpdatedcallback registration during SSR.The patch README links the equivalent upstream fix, vuepress/core#1722, and
explains when to remove the patch. Theme comments record the renamed options.
Regenerate the lockfile with npm 10.9.7 to include the missing nested Markdown
dependencies. Use
npm ciinci/doc.shso CI rejects an incomplete lockfileinstead of repairing it during the build. No heap increase is included.
Why did you make this change?
Related to #6644. VuePress stores content-update callbacks in a global set and
removes them on unmount. SSR does not run unmount hooks, so theme callbacks that
capture component state can retain previously rendered pages. These callbacks
are for DOM updates and do not need to be registered on the server.
With ESPnet's navigation structure and placeholder page bodies, the unpatched
upgrade exhausts a 4 GiB heap. Skipping SSR registration lets the same 1,715-page
fixture complete. A VuePress release containing the upstream fix should replace
the temporary patch.
Is your PR small enough?
Six files; the changes outside the generated lockfile are below 1,000 lines.
The dependency migration regenerates
package-lock.json, making the overalldiff exceed that limit. The lockfile and matching package/config changes need
to stay together; this needs an exception to the size guideline.
Additional Context
This builds on the dependency/config work in #6641 by @sw005320.
Latest local validation on macOS arm64, Node 22.22.0 and npm 10.9.7:
npm cifailure before regeneration, includingmissing
markdown-it,argparse,entities,linkify-it, anduc.micro.npm cinow succeeds and applies the SSR patch. The regenerated lockfile adds50 package records without changing any existing package versions.
npm audit: 0 vulnerabilities.bash -n ci/doc.shandgit diff --checkalso pass.NODE_OPTIONS=--max-old-space-size=4096 npm run docs:build: 1,715 fixture pagesrendered in 42.86 seconds, with the full VuePress build completing in 45.48 seconds.
The fixture uses ESPnet API page names and its menu generator, but placeholder
bodies. The reviewer separately verified the patch on the actual 1,904-page
corpus in their review.
The full documentation CI build passes on
e547048. Search, navigation, and code rendering still need a visual check of the generated output before merging.