fix: remove https:// prefix from origin to fix production build - #143
Conversation
Signed-off-by: Jonas Geiler <git@jonasgeiler.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/nodejs-website Please review the changes when you have a chance. Thank you! 🙏 |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Local builds generate incorrect HTTPS URLs instead of using the documented HTTP server.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
This PR updates origin handling to prevent duplicated https:// prefixes in generated URLs.
Changes:
- Removes schemes from preview and production origins.
- Local builds now incorrectly generate HTTPS URLs.
| File | Summary |
|---|---|
doc-kit.config.mjs |
Adjusts deployment origins, but requires conditional scheme handling for local HTTP builds. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bmuenzenmeyer
left a comment
bmuenzenmeyer
left a comment
There was a problem hiding this comment.
LGTM from poolside 🏊
|
Forgoing fast track. Broken prod. Working preview |
|
Seems resolved but mobile learn menu is empty for me. Will look tonight if no one else can |
Hmm I think I know what you mean but I don't know why that happens 🤔 No errors in the console for me... Unfortunately going to sleep soon. |
* fix: keep the scheme in `origin` so local builds use http nodejs#143 moved the scheme into `baseURL` as a hard-coded `https://`, so local builds now point their assets at https://localhost:3000, which `serve` doesn't answer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AkQ47hh78xyt4wWG9GVL5t * fix: hydrate the theme toggle Since the doc-kit 2 migration (nodejs#138) the navbar rendered the ui-components ThemeToggle directly. Only islands hydrate, so the button never opened. Use doc-kit's ThemeToggle island instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AkQ47hh78xyt4wWG9GVL5t * test: add Playwright smoke tests against Vercel previews Ports nodejs.org's playwright.yml and adds tests for asset URLs, the sitemap, navigation, search and the theme toggle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AkQ47hh78xyt4wWG9GVL5t * fixes playwright error * fix: hydrate the sidebar so mobile navigation works On small screens the sidebar collapses into a dropdown, which needs JavaScript to open. Since the doc-kit 2 migration (nodejs#138) only islands hydrate, so the dropdown rendered but did nothing. Register the sidebar as an island, like Authors, and cover small screens in the e2e tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AkQ47hh78xyt4wWG9GVL5t * Fix repository settings link Signed-off-by: Matt Cowley <me@mattcowley.co.uk> --------- Signed-off-by: Matt Cowley <me@mattcowley.co.uk> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Matt Cowley <me@mattcowley.co.uk>
I think #133 broke the production build. Not even the preview URL works for me there?
Looking at the devtools I see requests like this:
And it looks like there was a
https://prefix added accidentally to the production origin base URL, which then get's URL-sanitized, turning intohttps://https//nodejs.org/learn/assets/fonts/open-sans-latin-wght-normal.woff2and similar.If you compare the preview branches of #133:
https://nodejs-learn-git-fork-ojedajd-jo-improve-developm-2102dd-openjs.vercel.app/learn
and my preview branch here:
https://nodejs-learn-git-fork-jonasgeiler-patch-1-openjs.vercel.app/learn
you will see that it works again after reverting that change from #133.