Copilot commented on code in PR #362:
URL: https://github.com/apache/kvrocks-website/pull/362#discussion_r2941485521
##########
src/data/people.json:
##########
@@ -0,0 +1,204 @@
+{
+ "blogAuthors": {
+ "hulk": {
+ "name": "Hulk Lin",
+ "title": "Apache Kvrocks PMC Member",
+ "url": "https://github.com/git-hulk";,
+ "githubId": "git-hulk"
+ },
+ "vmihailenco": {
+ "name": "Vladimir Mihailenco",
+ "title": "Grumpy Gopher",
+ "url": "https://github.com/vmihailenco";,
+ "githubId": "vmihailenco"
+ },
+ "twice": {
+ "name": "PragmaTwice",
+ "title": "Apache Kvrocks PMC Member",
+ "url": "https://github.com/pragmatwice";,
+ "githubId": "pragmatwice"
+ }
+ },
+ "committers": [
+ {
+ "name": "Aleks Lozoviuk",
+ "apacheId": "aleksraiden",
+ "githubId": "aleksraiden",
+ "isPMC": true
+ },
+ {
+ "name": "Donghui Liu",
+ "apacheId": "alfejik",
+ "githubId": "Alfejik",
+ "isPMC": true
+ },
+ {
+ "name": "Agnik Misra",
+ "apacheId": "agnik",
+ "githubId": "Jitmisra",
+ "isPMC": false
+ },
+ {
+ "name": "Beihao Zhou",
+ "apacheId": "beihao",
+ "githubId": "Beihao-Zhou",
+ "isPMC": false
+ },
+ {
+ "name": "Binbin Zhu",
+ "apacheId": "binbin",
+ "githubId": "enjoy-binbin",
+ "isPMC": false
+ },
+ {
+ "name": "Brad Lee",
+ "apacheId": "bradlee",
+ "githubId": "smartlee",
+ "isPMC": false
+ },
+ {
+ "name": "Pengbo Cai",
+ "apacheId": "caipengbo",
+ "githubId": "caipengbo",
+ "isPMC": true
+ },
+ {
+ "name": "Liang Chen",
+ "apacheId": "chenliang613",
+ "githubId": "chenliang613",
+ "isPMC": true
+ },
+ {
+ "name": "Chris Zou",
+ "apacheId": "chriszou",
+ "githubId": "ChrisZMF",
+ "isPMC": false
+ },
+ {
+ "name": "Colin Chamber",
+ "apacheId": "colinchamber",
+ "githubId": "ColinChamber",
+ "isPMC": false
+ },
+ {
+ "name": "Rongxing Xiao",
+ "apacheId": "deemo",
+ "githubId": "yezhizi",
+ "isPMC": false
+ },
+ {
+ "name": "Edward Xu",
+ "apacheId": "edwardxu",
+ "githubId": "LindaSummer",
+ "isPMC": false
+ },
+ {
+ "name": "Xiaoqiao He",
+ "apacheId": "hexiaoqiao",
+ "githubId": "Hexiaoqiao",
+ "isPMC": true
+ },
+ {
+ "name": "Hulk Lin",
+ "apacheId": "hulk",
+ "githubId": "git-hulk",
+ "isPMC": true
+ },
+ {
+ "name": "Jean-Baptiste Onofré",
+ "apacheId": "jbonofre",
+ "githubId": "jbonofre",
+ "isPMC": true
+ },
+ {
+ "name": "Jean Lai",
+ "apacheId": "jeanbone",
+ "githubId": "jeanbone",
+ "isPMC": false
+ },
+ {
+ "name": "Ji Huayu",
+ "apacheId": "jihuayu",
+ "githubId": "jihuayu",
+ "isPMC": false
+ },
+ {
+ "name": "Miuyong Liu",
+ "apacheId": "karelrooted",
+ "githubId": "karelrooted",
+ "isPMC": false
+ },
+ {
+ "name": "Xuwei Fu",
+ "apacheId": "maplefu",
+ "githubId": "mapleFU",
+ "isPMC": true
+ },
+ {
+ "name": "Shang Xiong",
+ "apacheId": "shang",
+ "githubId": "shangxiaoxiong",
+ "isPMC": false
+ },
+ {
+ "name": "SiLe Zhou",
+ "apacheId": "silezhou",
+ "githubId": "PokIsemaine",
+ "isPMC": false
+ },
+ {
+ "name": "Xiaojun Yuan",
+ "apacheId": "sryanyuan",
+ "githubId": "sryanyuan",
+ "isPMC": false
+ },
+ {
+ "name": "Ruixiang Tan",
+ "apacheId": "tanruixiang",
+ "githubId": "tanruixiang",
+ "isPMC": false
+ },
+ {
+ "name": "Zili Chen",
+ "apacheId": "tison",
+ "githubId": "tisonkun",
+ "isPMC": true
+ },
+ {
+ "name": "Yaroslav Stepanchuk",
+ "apacheId": "torwig",
+ "githubId": "torwig",
+ "isPMC": true
+ },
+ {
+ "name": "Mingyang Liu",
+ "apacheId": "twice",
+ "githubId": "PragmaTwice",
Review Comment:
`people.committers` includes a `githubId` with different casing than the
same person in `blogAuthors` (e.g., `PragmaTwice` vs `pragmatwice`). The avatar
sync script writes files using `githubId` as the filename, so this can cause
duplicate downloads and (on case-insensitive file systems) filename
collisions/overwrites. Normalize `githubId` casing in this data (and ideally
keep one canonical value across authors/committers).
##########
scripts/sync-github-avatars.js:
##########
@@ -0,0 +1,175 @@
+#!/usr/bin/env node
+
+const fs = require("fs/promises");
+const path = require("path");
+const http = require("http");
+const https = require("https");
+
+const people = require("../src/data/people.json");
+
+const rootDir = path.resolve(__dirname, "..");
+const avatarDir = path.join(rootDir, "static/generated/avatars/github");
+const generatedAuthorsPath = path.join(rootDir, "blog/authors.generated.yml");
+const placeholderAvatarPath = "/img/avatar-placeholder.svg";
+const forceRefresh = /^(1|true)$/i.test(process.env.FORCE_SYNC_AVATARS || "");
+
+function avatarPublicPath(githubId) {
+ return `/generated/avatars/github/${githubId}.png`;
+}
+
+function getGithubIds() {
+ const ids = new Set();
+
+ for (const committer of people.committers) {
+ ids.add(committer.githubId);
+ }
+
+ for (const author of Object.values(people.blogAuthors)) {
+ if (author.githubId) {
+ ids.add(author.githubId);
+ }
+ }
+
+ return [...ids].sort((left, right) => left.localeCompare(right));
+}
Review Comment:
`getGithubIds()` collects raw `githubId` strings and later uses them as
filenames. Because GitHub logins are case-insensitive, mixed-case IDs can lead
to duplicate entries and (on case-insensitive file systems) avatar file
collisions/overwrites. Consider normalizing IDs (e.g., `toLowerCase()`) for the
cache key/filename, while optionally preserving original casing only for
display.
##########
src/components/Committers/index.tsx:
##########
@@ -59,7 +34,12 @@ export default function Committers(): JSX.Element {
.map(v => (
<tr key={v.name}>
<td><img width={64}
className={styles.contributorAvatar}
- src={`https://github.com/${v.githubId}.png`}
alt={v.name}/></td>
+ src={avatarPath(v.githubId)}
+ onError={(event) => {
+ event.currentTarget.onerror = null;
+ event.currentTarget.src =
placeholderAvatar;
+ }}
+ alt={v.name}/></td>
<td>{v.isPMC ? <b>{v.name}</b> : v.name}</td>
<td>{v.apacheId}</td>
<td><a target={"_blank"}
href={`https://github.com/${v.githubId}`}>{v.githubId}</a></td>
Review Comment:
Links opened with `target="_blank"` should set `rel="noopener noreferrer"`
to prevent reverse-tabnabbing and avoid leaking `window.opener` to the new page.
##########
src/components/Committers/index.tsx:
##########
@@ -59,7 +34,12 @@ export default function Committers(): JSX.Element {
.map(v => (
Review Comment:
`.sort(...)` mutates the underlying `committers` array in-place during
render. Since `committers` is module-level and sourced from
`people.committers`, this introduces a side effect and does unnecessary work on
every render. Use a non-mutating pattern (e.g., sort a copied array or memoize
the sorted list) to keep render pure.
##########
scripts/sync-github-avatars.js:
##########
@@ -0,0 +1,175 @@
+#!/usr/bin/env node
+
+const fs = require("fs/promises");
+const path = require("path");
+const http = require("http");
+const https = require("https");
+
+const people = require("../src/data/people.json");
+
+const rootDir = path.resolve(__dirname, "..");
+const avatarDir = path.join(rootDir, "static/generated/avatars/github");
+const generatedAuthorsPath = path.join(rootDir, "blog/authors.generated.yml");
+const placeholderAvatarPath = "/img/avatar-placeholder.svg";
+const forceRefresh = /^(1|true)$/i.test(process.env.FORCE_SYNC_AVATARS || "");
+
+function avatarPublicPath(githubId) {
+ return `/generated/avatars/github/${githubId}.png`;
+}
+
+function getGithubIds() {
+ const ids = new Set();
+
+ for (const committer of people.committers) {
+ ids.add(committer.githubId);
+ }
+
+ for (const author of Object.values(people.blogAuthors)) {
+ if (author.githubId) {
+ ids.add(author.githubId);
+ }
+ }
+
+ return [...ids].sort((left, right) => left.localeCompare(right));
+}
+
+async function exists(filePath) {
+ try {
+ await fs.access(filePath);
+ return true;
+ } catch {
+ return false;
+ }
+}
+
+function download(url, redirects = 0) {
+ if (redirects > 5) {
+ return Promise.reject(new Error(`Too many redirects for ${url}`));
+ }
+
+ const client = url.startsWith("https:") ? https : http;
+
+ return new Promise((resolve, reject) => {
+ const request = client.get(
+ url,
+ {
+ headers: {
+ "user-agent": "kvrocks-website-avatar-sync",
+ },
+ },
+ (response) => {
+ const { statusCode = 0, headers } = response;
+
+ if (statusCode >= 300 && statusCode < 400 && headers.location) {
+ response.resume();
+ const nextUrl = new URL(headers.location, url).toString();
+ resolve(download(nextUrl, redirects + 1));
+ return;
+ }
+
+ if (statusCode !== 200) {
+ response.resume();
+ reject(new Error(`Unexpected status ${statusCode} for ${url}`));
+ return;
+ }
+
+ const chunks = [];
+ response.on("data", (chunk) => chunks.push(chunk));
+ response.on("end", () => resolve(Buffer.concat(chunks)));
+ }
+ );
+
+ request.on("error", reject);
+ });
+}
Review Comment:
`download()` does not set any request/response timeout, so `yarn
start`/`yarn build` (which run this script via prestart/prebuild) can hang
indefinitely on stalled connections. Add a timeout and abort/destroy the
request (and consider a per-download retry/backoff) so builds fail fast and
predictably.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
