feat(changelogs): redesign changelog page with combined feed and commit view#664
Conversation
…it view - Replace two-column FeedItems grid with single full-width CombinedFeedItems that merges both feeds sorted newest-first with per-entry stream badges - Add CopyButton (clipboard icon, hover fade, 1.5s confirmation) to each entry - Add extractCommits() to parse <h3>Commits</h3> HTML table into structured rows with hash links, PR-linked subjects, and author columns per card - Center PackageSummary boxes (max 420px, LTS left / Bluefin right) - Update formatReleaseTitle() to keep stream prefix; remove GTS branch - Remove GTS from update-driver-versions.js and build-metrics.mjs Assisted-by: Claude Sonnet 4.6 via OpenCode
There was a problem hiding this comment.
Code Review
This pull request redesigns the changelog page by combining the Bluefin and Bluefin LTS release feeds into a single view, and adds a detailed commit list for each release. The changes are a significant improvement to the user experience. My review focuses on the implementation of the new CombinedFeedItems component. I've identified a critical security vulnerability related to the use of dangerouslySetInnerHTML and a high-severity design issue that impacts the reusability of the new component. Addressing these will improve the robustness and security of the new page.
| const extractCommits = (content: string): CommitEntry[] => { | ||
| if (!content) return []; | ||
|
|
||
| // Find the <h3>Commits</h3> section (feed content is already HTML, not markdown) | ||
| const commitsMatch = content.match( | ||
| /<h3>Commits<\/h3>\s*<table[\s\S]*?<tbody>([\s\S]*?)<\/tbody>/, | ||
| ); | ||
| if (!commitsMatch) return []; | ||
|
|
||
| const tbody = commitsMatch[1]; | ||
| const rows = tbody.match(/<tr>([\s\S]*?)<\/tr>/g); | ||
| if (!rows) return []; | ||
|
|
||
| const commits: CommitEntry[] = []; | ||
| for (const row of rows) { | ||
| // Hash cell: <td><strong><a href="URL">HASH</a></strong></td> | ||
| const hashMatch = row.match( | ||
| /<td><strong><a href="([^"]+)">([^<]+)<\/a><\/strong><\/td>/, | ||
| ); | ||
| // Subject cell: second <td> — may contain nested HTML | ||
| const cells = row.match(/<td>([\s\S]*?)<\/td>/g); | ||
| // Author cell: third <td> | ||
| if (!hashMatch || !cells || cells.length < 3) continue; | ||
|
|
||
| const hashUrl = hashMatch[1]; | ||
| const hash = hashMatch[2]; | ||
| // Strip outer <td>…</td> tags from subject cell | ||
| const subject = cells[1].replace(/^<td>|<\/td>$/g, ""); | ||
| const author = cells[2].replace(/<[^>]+>/g, "").trim(); | ||
|
|
||
| commits.push({ hash, hashUrl, subject, author }); | ||
| } | ||
| return commits; | ||
| }; |
There was a problem hiding this comment.
The extractCommits function uses regular expressions to parse HTML from the release content. This is fragile and can easily break if the HTML structure of the release notes changes. A more robust approach is to use the browser's DOMParser API to parse the HTML string into a DOM tree and then traverse it to extract the required information.
More importantly, the extracted subject (which is raw HTML) is later rendered using dangerouslySetInnerHTML without any sanitization. This creates a potential Cross-Site Scripting (XSS) vulnerability. While the source (GitHub releases) is likely trusted, it is a security best practice to always sanitize any HTML before rendering it with dangerouslySetInnerHTML. An attacker who finds a way to inject malicious content into the release notes could compromise users of your site.
I recommend refactoring extractCommits to use DOMParser and then sanitizing the subject HTML with a library like dompurify before passing it to dangerouslySetInnerHTML. You will need to add dompurify as a dependency (npm install dompurify @types/dompurify) and import it where needed.
const extractCommits = (content: string): CommitEntry[] => {
if (!content) return [];
// DOMParser is safe to use here as this component runs on the client.
try {
const parser = new DOMParser();
const doc = parser.parseFromString(content, "text/html");
const commitsHeading = Array.from(doc.querySelectorAll('h3')).find(
(h3) => h3.textContent?.trim() === 'Commits'
);
if (!commitsHeading) return [];
const table = commitsHeading.nextElementSibling;
if (!table || table.tagName !== 'TABLE') return [];
const rows = Array.from(table.querySelectorAll('tbody tr'));
const commits: CommitEntry[] = [];
for (const row of rows) {
const cells = row.querySelectorAll('td');
if (cells.length < 3) continue;
const hashLink = cells[0].querySelector('a');
if (!hashLink) continue;
commits.push({
hashUrl: hashLink.href,
hash: hashLink.textContent || '',
// Sanitize innerHTML before using it with dangerouslySetInnerHTML
subject: DOMPurify.sanitize(cells[1].innerHTML),
author: cells[2].textContent?.trim() || '',
});
}
return commits;
} catch (error) {
console.error("Failed to parse commits from HTML", error);
return [];
}
};
| // Call useStoredFeed for each feed (hooks must be called unconditionally at top level) | ||
| const feedDataLts: ParsedFeed = useStoredFeed( | ||
| feeds[0]?.feedId ?? "bluefinLtsReleases", | ||
| ); | ||
| const feedDataStable: ParsedFeed = useStoredFeed( | ||
| feeds[1]?.feedId ?? "bluefinReleases", | ||
| ); | ||
| const rawFeeds = [feedDataLts, feedDataStable]; |
There was a problem hiding this comment.
The CombinedFeedItems component is not designed to be scalable or reusable. It's hardcoded to fetch and combine exactly two feeds by calling useStoredFeed directly. This approach is brittle and prevents easily combining more than two feeds or different feeds without modifying the component's internal logic.
A better design would be to move the data fetching logic to the parent component (CommunityFeeds.tsx) and have CombinedFeedItems be a presentational component that accepts the fetched data via props. This would make CombinedFeedItems more reusable, easier to test, and would decouple data fetching from presentation.
Example of a better approach:
// In CommunityFeeds.tsx
const CommunityFeeds: React.FC = () => {
const feedDataLts = useStoredFeed("bluefinLtsReleases");
const feedDataStable = useStoredFeed("bluefinReleases");
const feedsWithData = [
{ data: feedDataLts, label: "Bluefin LTS", feedId: "bluefinLtsReleases" },
{ data: feedDataStable, label: "Bluefin", feedId: "bluefinReleases", filter: (item) => item.title.startsWith("stable-") }
];
return (
// ...
<CombinedFeedItems
title="Release Changelogs"
feeds={feedsWithData}
maxItems={20}
/>
// ...
);
}
// In FeedItems.tsx, CombinedFeedItems would be refactored to accept `feeds` with `data`
// and would no longer call `useStoredFeed`.
Summary
Test Plan