Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #617 +/- ##
==========================================
- Coverage 77.08% 77.06% -0.03%
==========================================
Files 153 153
Lines 13749 13755 +6
Branches 1111 1111
==========================================
+ Hits 10599 10600 +1
- Misses 3146 3151 +5
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
| File | Base | Head | Diff |
|---|---|---|---|
orama-db.json |
8.27 MB | 8.27 MB | -1.41 KB (-0.02%) |
web Generator
| File | Base | Head | Diff |
|---|---|---|---|
styles.css |
137.49 KB | 139.42 KB | +1.93 KB (+1.40%) |
addons.html |
262.56 KB | 262.57 KB | +11.00 B (+0.00%) |
assert.html |
330.00 KB | 330.01 KB | +11.00 B (+0.00%) |
async_context.html |
187.58 KB | 187.59 KB | +11.00 B (+0.01%) |
async_hooks.html |
159.71 KB | 159.72 KB | +11.00 B (+0.01%) |
buffer.html |
905.72 KB | 905.73 KB | +11.00 B (+0.00%) |
child_process.html |
381.70 KB | 381.71 KB | +11.00 B (+0.00%) |
cli.html |
495.11 KB | 495.13 KB | +11.00 B (+0.00%) |
cluster.html |
196.30 KB | 196.31 KB | +11.00 B (+0.01%) |
console.html |
146.77 KB | 146.78 KB | +11.00 B (+0.01%) |
crypto.html |
1.09 MB | 1.09 MB | +11.00 B (+0.00%) |
debugger.html |
54.22 KB | 54.24 KB | +11.00 B (+0.02%) |
deprecations.html |
505.31 KB | 505.32 KB | +11.00 B (+0.00%) |
dgram.html |
194.37 KB | 194.38 KB | +11.00 B (+0.01%) |
diagnostics_channel.html |
303.42 KB | 303.43 KB | +11.00 B (+0.00%) |
dns.html |
295.81 KB | 295.82 KB | +11.00 B (+0.00%) |
documentation.html |
38.04 KB | 38.05 KB | +11.00 B (+0.03%) |
domain.html |
105.10 KB | 105.12 KB | +11.00 B (+0.01%) |
embedding.html |
59.43 KB | 59.44 KB | +11.00 B (+0.02%) |
environment_variables.html |
44.04 KB | 44.05 KB | +11.00 B (+0.02%) |
errors.html |
461.18 KB | 461.19 KB | +11.00 B (+0.00%) |
esm.html |
155.33 KB | 155.34 KB | +11.00 B (+0.01%) |
events.html |
455.27 KB | 455.28 KB | +11.00 B (+0.00%) |
fs.html |
1.43 MB | 1.43 MB | +11.00 B (+0.00%) |
globals.html |
230.37 KB | 230.38 KB | +11.00 B (+0.00%) |
http.html |
746.17 KB | 746.18 KB | +11.00 B (+0.00%) |
http2.html |
768.59 KB | 768.60 KB | +11.00 B (+0.00%) |
https.html |
150.35 KB | 150.36 KB | +11.00 B (+0.01%) |
index.html |
36.25 KB | 36.26 KB | +11.00 B (+0.03%) |
inspector.html |
171.38 KB | 171.39 KB | +11.00 B (+0.01%) |
intl.html |
58.14 KB | 58.15 KB | +11.00 B (+0.02%) |
module.html |
326.66 KB | 326.67 KB | +11.00 B (+0.00%) |
modules.html |
178.73 KB | 178.74 KB | +11.00 B (+0.01%) |
n-api.html |
807.25 KB | 807.26 KB | +11.00 B (+0.00%) |
net.html |
382.29 KB | 382.30 KB | +11.00 B (+0.00%) |
os.html |
142.73 KB | 142.74 KB | +11.00 B (+0.01%) |
packages.html |
154.65 KB | 154.66 KB | +11.00 B (+0.01%) |
path.html |
139.26 KB | 139.27 KB | +11.00 B (+0.01%) |
perf_hooks.html |
381.49 KB | 381.50 KB | +11.00 B (+0.00%) |
permissions.html |
55.14 KB | 55.15 KB | +11.00 B (+0.02%) |
process.html |
676.15 KB | 676.16 KB | +11.00 B (+0.00%) |
punycode.html |
63.19 KB | 63.20 KB | +11.00 B (+0.02%) |
querystring.html |
63.85 KB | 63.86 KB | +11.00 B (+0.02%) |
quic.html |
358.98 KB | 358.99 KB | +11.00 B (+0.00%) |
readline.html |
252.17 KB | 252.19 KB | +11.00 B (+0.00%) |
repl.html |
183.30 KB | 183.31 KB | +11.00 B (+0.01%) |
report.html |
175.92 KB | 175.93 KB | +11.00 B (+0.01%) |
single-executable-applications.html |
106.95 KB | 106.96 KB | +11.00 B (+0.01%) |
sqlite.html |
268.08 KB | 268.09 KB | +11.00 B (+0.00%) |
stream.html |
855.31 KB | 855.32 KB | +11.00 B (+0.00%) |
stream_iter.html |
309.03 KB | 309.04 KB | +11.00 B (+0.00%) |
string_decoder.html |
55.39 KB | 55.41 KB | +11.00 B (+0.02%) |
synopsis.html |
42.67 KB | 42.68 KB | +11.00 B (+0.03%) |
test.html |
766.72 KB | 766.73 KB | +11.00 B (+0.00%) |
timers.html |
133.51 KB | 133.52 KB | +11.00 B (+0.01%) |
tls.html |
373.79 KB | 373.80 KB | +11.00 B (+0.00%) |
tracing.html |
84.16 KB | 84.17 KB | +11.00 B (+0.01%) |
tty.html |
95.29 KB | 95.30 KB | +11.00 B (+0.01%) |
typescript.html |
53.47 KB | 53.48 KB | +11.00 B (+0.02%) |
url.html |
347.68 KB | 347.69 KB | +11.00 B (+0.00%) |
util.html |
691.89 KB | 691.90 KB | +11.00 B (+0.00%) |
v8.html |
321.09 KB | 321.10 KB | +11.00 B (+0.00%) |
vm.html |
370.80 KB | 370.81 KB | +11.00 B (+0.00%) |
wasi.html |
69.37 KB | 69.38 KB | +11.00 B (+0.02%) |
webcrypto.html |
522.00 KB | 522.01 KB | +11.00 B (+0.00%) |
webstreams.html |
355.74 KB | 355.75 KB | +11.00 B (+0.00%) |
worker_threads.html |
365.47 KB | 365.48 KB | +11.00 B (+0.00%) |
zlib.html |
290.24 KB | 290.25 KB | +11.00 B (+0.00%) |
zlib_iter.html |
88.06 KB | 88.07 KB | +11.00 B (+0.01%) |
There was a problem hiding this comment.
Pull request overview
This PR implements an announcement banner component that fetches configuration from a remote JSON endpoint to display global and version-specific banners on the API documentation pages. The banners support date-based activation/deactivation and can include links to relevant resources.
Changes:
- Added
AnnouncementBannercomponent that asynchronously fetches and displays banners from a remote config - Implemented
isBannerActiveutility function to filter banners based on date ranges - Added
versionMajorandremoteConfigfields to static data passed from server to client
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/generators/web/index.mjs |
Adds remote config URL to web generator default configuration |
src/generators/web/types.d.ts |
Adds remoteConfig type definition for the web generator |
src/generators/web/utils/data.mjs |
Extracts version major and remote config URL from server config to client static data |
src/generators/web/constants.mjs |
Registers AnnouncementBanner component in JSX imports map |
src/generators/jsx-ast/utils/buildContent.mjs |
Adds AnnouncementBanner to document layout above navigation bar |
src/generators/web/ui/utils/banner.mjs |
Implements date range checking logic for banner activation |
src/generators/web/ui/utils/__tests__/banner.test.mjs |
Comprehensive tests for banner date range logic |
src/generators/web/ui/components/AnnouncementBanner/types.d.ts |
Type definitions for banner entries and remote config structure |
src/generators/web/ui/components/AnnouncementBanner/index.jsx |
Main component that fetches config and renders active banners |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div> | ||
| {banners.map(banner => ( | ||
| <Banner key={banner.link} type={banner.type}> | ||
| {banner.link ? <a href={banner.link}>{banner.text}</a> : banner.text} |
There was a problem hiding this comment.
External URLs from the remote config are used directly in href attributes without validation. While React/Preact provides some protection against javascript: protocol URLs, there's still a potential security risk if the remote config source is compromised. Consider validating that banner.link uses an approved protocol (https://) or implementing an allowlist of acceptable domains to ensure only trusted URLs are rendered. This is especially important since the config is fetched from an external source.
There was a problem hiding this comment.
@avivkeller @ovflowd what do you think? is this necessary? We don't do this on nodejs website
|
@araujogui please be aware this must 100% be a client-side only component. Since the value of this comes from being able to fetch this at page load, as docs are built only once for each version. |
|
(this also means that this spec cannot ever change or needs to be forever retroactively compatible) |
It is |
uh, that also means we can't change the config url, so we should already create the config file on nodejs/node? |
Yeah... So we need to be very careful here 😓 |
We already have a |
We can use site.json, but note that it is not publicly available, the link would either need to be through raw.githubusercontent.com or we'd need to move the file to be publicly available. |
I think raw.githubusercontent.com is good enough |
IDK about that... |
|
I'd be fine with using raw.githubusercontent.com at build time (like we do via nodevu on the site), but I don't think we should be reliant on hitting it client-side every time someone loads a page on the site, that feels like abuse of GitHub's service. Can we have a JSON file accessible via the site, or an API endpiont? |
We won't be loading this at build-time, cause it is supposed to be a client-side only, so it would hit indeed on every page load. I don't think raw.githubusercontent.com has cache headers so it would not be good. |
Okay, maybe we could expose |
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
Let's put a block on this for now as this needs to be done thoughtfully
PR SummaryMedium Risk Overview Extends generated Reviewed by Cursor Bugbot for commit 7e2aeca. Bugbot is set up for automated code reviews on this repo. Configure here. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/generators/web/ui/components/AnnouncementBanner/AnnouncementBanner.jsx
Show resolved
Hide resolved
src/generators/web/ui/components/AnnouncementBanner/AnnouncementBanner.jsx
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Missing
res.okcheck allows error responses through- Added an explicit
res.okguard inloadBannersto return an empty array for non-ok HTTP responses and strengthened the test to include banner data in a non-ok response.
- Added an explicit
- ✅ Fixed: JSDoc uses
T[]instead ofArray<T>syntax- Updated the
@returnsJSDoc inloadBannersfromBannerEntry[]shorthand to the requiredArray<BannerEntry>form.
- Updated the
- ✅ Fixed: Hardcoded external URL belongs in constants file
- Moved the default remote config URL into
src/generators/web/constants.mjsasDEFAULT_REMOTE_CONFIG_URLand referenced it fromsrc/generators/web/index.mjs.
- Moved the default remote config URL into
Or push these changes by commenting:
@cursor push 38c45a2ce9
Preview (38c45a2ce9)
diff --git a/src/generators/web/constants.mjs b/src/generators/web/constants.mjs
--- a/src/generators/web/constants.mjs
+++ b/src/generators/web/constants.mjs
@@ -2,6 +2,7 @@
import { fileURLToPath } from 'node:url';
export const ROOT = dirname(fileURLToPath(import.meta.url));
+export const DEFAULT_REMOTE_CONFIG_URL = 'https://nodejs.org/site.json';
/**
* @typedef {Object} JSXImportConfig
diff --git a/src/generators/web/index.mjs b/src/generators/web/index.mjs
--- a/src/generators/web/index.mjs
+++ b/src/generators/web/index.mjs
@@ -2,6 +2,7 @@
import { join } from 'node:path';
+import { DEFAULT_REMOTE_CONFIG_URL } from './constants.mjs';
import { GITHUB_EDIT_URL } from '../../utils/configuration/templates.mjs';
import { createLazyGenerator } from '../../utils/generators.mjs';
@@ -33,7 +34,7 @@
title: '{project} v{version} Documentation',
editURL: `${GITHUB_EDIT_URL}/doc/api{path}.md`,
pageURL: '{baseURL}/latest-{version}/api{path}.html',
- remoteConfig: 'https://nodejs.org/site.json',
+ remoteConfig: DEFAULT_REMOTE_CONFIG_URL,
imports: {
'#theme/Logo': '@node-core/ui-components/Common/NodejsLogo',
'#theme/Navigation': join(import.meta.dirname, './ui/components/NavBar'),
diff --git a/src/generators/web/ui/components/AnnouncementBanner/__tests__/loadBanners.test.mjs b/src/generators/web/ui/components/AnnouncementBanner/__tests__/loadBanners.test.mjs
--- a/src/generators/web/ui/components/AnnouncementBanner/__tests__/loadBanners.test.mjs
+++ b/src/generators/web/ui/components/AnnouncementBanner/__tests__/loadBanners.test.mjs
@@ -26,8 +26,9 @@
});
it('returns an empty array on non-ok response', async t => {
+ const banner = { text: 'Global banner', type: 'warning' };
t.mock.method(global, 'fetch', () =>
- Promise.resolve(makeResponse({}, false))
+ Promise.resolve(makeResponse({ index: banner }, false))
);
const result = await loadBanners('https://example.com/site.json', null);
diff --git a/src/generators/web/ui/components/AnnouncementBanner/loadBanners.mjs b/src/generators/web/ui/components/AnnouncementBanner/loadBanners.mjs
--- a/src/generators/web/ui/components/AnnouncementBanner/loadBanners.mjs
+++ b/src/generators/web/ui/components/AnnouncementBanner/loadBanners.mjs
@@ -7,7 +7,7 @@
*
* @param {string | undefined} remoteConfig
* @param {number | null} versionMajor
- * @returns {Promise<import('./types.d.ts').BannerEntry[]>}
+ * @returns {Promise<Array<import('./types.d.ts').BannerEntry>>}
*/
export const loadBanners = async (remoteConfig, versionMajor) => {
if (!remoteConfig) {
@@ -16,6 +16,9 @@
try {
const res = await fetch(remoteConfig);
+ if (!res.ok) {
+ return [];
+ }
/** @type {import('./types.d.ts').RemoteConfig} */
const { websiteBanners = {} } = await res.json();This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f97c27d. Configure here.
Description
Create announcements banner component
Validation
Related Issues
Fixes #327
Check List
node --run testand all tests passed.node --run format&node --run lint.