Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/generators/jsx-ast/utils/buildContent.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ export const createDocumentLayout = (
remark
) =>
createTree('root', [
createJSXElement(JSX_IMPORTS.AnnouncementBanner.name),
createJSXElement(JSX_IMPORTS.NavBar.name),
createJSXElement(JSX_IMPORTS.Article.name, {
children: [
Expand Down
4 changes: 4 additions & 0 deletions src/generators/web/constants.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ export const ROOT = dirname(fileURLToPath(import.meta.url));
* An object containing mappings for various JSX components to their import paths.
*/
export const JSX_IMPORTS = {
AnnouncementBanner: {
name: 'AnnouncementBanner',
source: resolve(ROOT, './ui/components/AnnouncementBanner'),
},
NavBar: {
name: 'NavBar',
source: resolve(ROOT, './ui/components/NavBar'),
Expand Down
2 changes: 2 additions & 0 deletions src/generators/web/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ export default {
imports: {
'#config/Logo': '@node-core/ui-components/Common/NodejsLogo',
},
remoteConfig:
'https://gist.githubusercontent.com/araujogui/8ea72ffaf574f58fca1482e764e8b5c8/raw/16af51e4efbf37da7b6aff9b7e5dd967d955aacf/api-docs.config.json',
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded Gist URL includes a specific commit hash in the raw URL. This means the configuration is pinned to a specific version and won't automatically update when the Gist is modified. If the intent is to fetch the latest version dynamically, the URL should use the format without the commit hash (e.g., ending with /raw/api-docs.config.json). If the commit hash is intentional for stability, consider documenting why this specific version is pinned and how it should be updated.

Suggested change
'https://gist.githubusercontent.com/araujogui/8ea72ffaf574f58fca1482e764e8b5c8/raw/16af51e4efbf37da7b6aff9b7e5dd967d955aacf/api-docs.config.json',
'https://gist.githubusercontent.com/araujogui/8ea72ffaf574f58fca1482e764e8b5c8/raw/api-docs.config.json',

Copilot uses AI. Check for mistakes.
Comment on lines +37 to +38
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to issue #327, the configuration should reside "on this repository or nodejs/node" as a static api-docs.config.json file, but this implementation uses a GitHub Gist URL instead. Consider whether this temporary Gist URL is intentional for testing/development, or if the final implementation should point to a file hosted in the official repositories. If this is a development URL, it should be documented or replaced before merging.

Copilot uses AI. Check for mistakes.
Comment on lines +37 to +38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a banner config defined anywhere, so, in the meantime, can we have it read from nodejs.org's site.json (or whatever file stores the main sites' banner)

},

/**
Expand Down
1 change: 1 addition & 0 deletions src/generators/web/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export type Generator = GeneratorMetadata<
templatePath: string;
title: string;
imports: Record<string, string>;
remoteConfig: string | null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
remoteConfig: string | null;
remoteConfig: string;

},
Generate<Array<JSXContent>, AsyncGenerator<{ html: string; css: string }>>
>;
69 changes: 69 additions & 0 deletions src/generators/web/ui/components/AnnouncementBanner/index.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { ArrowUpRightIcon } from '@heroicons/react/24/outline';
import Banner from '@node-core/ui-components/Common/Banner';
import { useEffect, useState } from 'preact/hooks';

import { STATIC_DATA } from '../../constants.mjs';
import { isBannerActive } from '../../utils/banner.mjs';

/** @import { BannerEntry, RemoteConfig } from './types.d.ts' */

/**
* Asynchronously fetches and displays announcement banners from the remote config.
* Global banners are rendered above version-specific ones.
* Non-blocking: silently ignores fetch/parse failures.
*/
export default () => {
const [banners, setBanners] = useState(/** @type {BannerEntry[]} */ ([]));

useEffect(() => {
const { remoteConfig, versionMajor } = STATIC_DATA;

if (!remoteConfig) {
return;
}

fetch(remoteConfig, {
signal: AbortSignal.timeout(2500),
})
.then(async res => {
if (!res.ok) {
return;
}

/** @type {RemoteConfig} */
const config = await res.json();

const active = [];

const globalBanner = config.global?.banner;
if (globalBanner && isBannerActive(globalBanner)) {
active.push(globalBanner);
}

const versionBanner = config[`v${versionMajor}`]?.banner;
if (versionBanner && isBannerActive(versionBanner)) {
active.push(versionBanner);
Comment on lines +43 to +45
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The versionMajor is being used without a null check when constructing the version key. If config.version?.major is null (when version is not available), the template literal will create a key "vnull" which doesn't match the expected format "v24" from the issue description. Consider early returning or handling the null case explicitly before attempting to fetch version-specific banners.

Suggested change
const versionBanner = config[`v${versionMajor}`]?.banner;
if (versionBanner && isBannerActive(versionBanner)) {
active.push(versionBanner);
if (versionMajor != null) {
const versionBanner = config[`v${versionMajor}`]?.banner;
if (versionBanner && isBannerActive(versionBanner)) {
active.push(versionBanner);
}

Copilot uses AI. Check for mistakes.
}

setBanners(active);
})
.catch(error => {
console.error(error);
});
}, []);

if (!banners.length) {
return null;
}

return (
<div>
{banners.map(banner => (
<Banner key={banner.link} type={banner.type}>
Comment on lines +61 to +62
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key used for banners without links will be undefined, which can cause React reconciliation issues. When there are multiple banners without links, they will all have the same key (undefined), leading to incorrect updates and potential bugs in the banner display. Consider using the banner text or index as a fallback key, or use a combination of fields to ensure uniqueness. For example: key={banner.link || banner.text} or key={banner.link || ${banner.text}-${index}}.

Suggested change
{banners.map(banner => (
<Banner key={banner.link} type={banner.type}>
{banners.map((banner, index) => (
<Banner key={banner.link || `${banner.text}-${index}`} type={banner.type}>

Copilot uses AI. Check for mistakes.
{banner.link ? <a href={banner.link}>{banner.text}</a> : banner.text}
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
{banner.link && <ArrowUpRightIcon />}
Comment on lines +63 to +64
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ArrowUpRightIcon typically indicates an external link that opens in a new tab, but the anchor tag is missing target="_blank" and rel="noopener noreferrer" attributes. If the intention is for these links to open in a new tab (as the icon suggests), add target="_blank". For security and accessibility, also include rel="noopener noreferrer" to prevent the linked page from accessing window.opener, and consider adding an aria-label to indicate the link opens externally.

Suggested change
{banner.link ? <a href={banner.link}>{banner.text}</a> : banner.text}
{banner.link && <ArrowUpRightIcon />}
{banner.link ? (
<a
href={banner.link}
target="_blank"
rel="noopener noreferrer"
aria-label={`${banner.text} (opens in a new tab)`}
>
{banner.text}
</a>
) : (
banner.text
)}
{banner.link && <ArrowUpRightIcon aria-hidden="true" />}

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this can go within the above line

</Banner>
))}
</div>
Comment on lines +60 to +67
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wrapper div element lacks semantic meaning. Consider using a more semantic HTML element like <aside> or adding appropriate ARIA attributes to indicate this is an announcements region. For example, <div role="region" aria-label="Announcements"> would improve accessibility by clearly identifying the purpose of this section to screen readers.

Suggested change
<div>
{banners.map(banner => (
<Banner key={banner.link} type={banner.type}>
{banner.link ? <a href={banner.link}>{banner.text}</a> : banner.text}
{banner.link && <ArrowUpRightIcon />}
</Banner>
))}
</div>
<aside aria-label="Announcements">
{banners.map(banner => (
<Banner key={banner.link} type={banner.type}>
{banner.link ? <a href={banner.link}>{banner.text}</a> : banner.text}
{banner.link && <ArrowUpRightIcon />}
</Banner>
))}
</aside>

Copilot uses AI. Check for mistakes.
);
};
11 changes: 11 additions & 0 deletions src/generators/web/ui/components/AnnouncementBanner/types.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import type { BannerProps } from '@node-core/ui-components/Common/Banner';

export type BannerEntry = {
startDate?: string;
endDate?: string;
text: string;
link?: string;
type?: BannerProps['type'];
};

export type RemoteConfig = Record<string, { banner?: BannerEntry } | undefined>;
63 changes: 63 additions & 0 deletions src/generators/web/ui/utils/__tests__/banner.test.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import assert from 'node:assert/strict';
import { describe, it } from 'node:test';

import { isBannerActive } from '../banner.mjs';

const PAST = new Date(Date.now() - 86_400_000).toISOString(); // yesterday
const FUTURE = new Date(Date.now() + 86_400_000).toISOString(); // tomorrow

const banner = (overrides = {}) => ({
text: 'Test banner',
...overrides,
});

describe('isBannerActive', () => {
describe('no startDate, no endDate', () => {
it('is always active', () => {
assert.equal(isBannerActive(banner()), true);
});
});

describe('startDate only', () => {
it('is active when startDate is in the past', () => {
assert.equal(isBannerActive(banner({ startDate: PAST })), true);
});

it('is not active when startDate is in the future', () => {
assert.equal(isBannerActive(banner({ startDate: FUTURE })), false);
});
});

describe('endDate only', () => {
it('is active when endDate is in the future', () => {
assert.equal(isBannerActive(banner({ endDate: FUTURE })), true);
});

it('is not active when endDate is in the past', () => {
assert.equal(isBannerActive(banner({ endDate: PAST })), false);
});
});

describe('startDate and endDate', () => {
it('is active when now is within the range', () => {
assert.equal(
isBannerActive(banner({ startDate: PAST, endDate: FUTURE })),
true
);
});

it('is not active when now is before the range', () => {
assert.equal(
isBannerActive(banner({ startDate: FUTURE, endDate: FUTURE })),
false
);
});

it('is not active when now is after the range', () => {
assert.equal(
isBannerActive(banner({ startDate: PAST, endDate: PAST })),
false
);
});
});
});
20 changes: 20 additions & 0 deletions src/generators/web/ui/utils/banner.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/** @import { BannerEntry } from '../components/AnnouncementBanner/types' */

/**
* Checks whether a banner should be displayed based on its date range.
* Both `startDate` and `endDate` are optional; if omitted the banner is
* considered open-ended in that direction.
*
* @param {BannerEntry} banner
* @returns {boolean}
*/
export const isBannerActive = banner => {
const now = Date.now();
if (banner.startDate && now < new Date(banner.startDate).getTime()) {
return false;
}
if (banner.endDate && now > new Date(banner.endDate).getTime()) {
return false;
}
return true;
};
2 changes: 2 additions & 0 deletions src/generators/web/utils/data.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ export const createStaticData = () => {
shikiDisplayNameMap,
title: config.title,
repository: config.repository,
versionMajor: config.version?.major ?? null,
remoteConfig: config.remoteConfig ?? null,
Comment on lines +35 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of this data is for components that can't accept the data as properties. Can we instead pass these values as properties to the component, which produces a smaller bundle?

};
};

Expand Down
Loading