Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new Benchmark component to the home page, showcasing performance metrics for various build tools across different workloads. The feedback identifies several improvement opportunities: localizing hardcoded strings within the benchmark data, enhancing accessibility and SEO by replacing a programmatic window.open call with a standard anchor tag, and cleaning up unused translation keys in the i18n files.
| modules: "1,000 components", | ||
| startup: { tool: "Rsbuild", value: "1529ms" }, | ||
| build: { tool: "Vite (Rolldown)", value: "628ms" }, | ||
| hmr: { tool: "Rspack CLI", value: "107ms" }, | ||
| }, | ||
| { | ||
| caseName: "react-5k", | ||
| modules: "5,000 components", | ||
| startup: { tool: "Rsbuild", value: "1346ms" }, | ||
| build: { tool: "Vite (Rolldown)", value: "1921ms" }, | ||
| hmr: { tool: "Farm", value: "85ms" }, | ||
| }, | ||
| { | ||
| caseName: "react-10k", | ||
| modules: "10,000 components", |
There was a problem hiding this comment.
| <Button | ||
| size="lg" | ||
| variant="outline" | ||
| className="h-auto w-full justify-between rounded-[1.75rem] border-white/10 bg-white/5 px-6 py-5 text-left text-base text-foreground hover:bg-white/10" | ||
| onClick={() => | ||
| window.open( | ||
| "https://github.com/utooland/build-tools-performance", | ||
| "_blank", | ||
| ) | ||
| } | ||
| > | ||
| <span>{t.benchmark.cta}</span> | ||
| <ArrowUpRight className="h-5 w-5 shrink-0" /> | ||
| </Button> |
There was a problem hiding this comment.
Using onClick with window.open for an external link is not ideal for accessibility and SEO. It is better to use a standard anchor tag (<a>). Since you are using a UI Button component, you can use the asChild pattern to render it as a link while preserving the button's styles.
<Button
asChild
size="lg"
variant="outline"
className="h-auto w-full justify-between rounded-[1.75rem] border-white/10 bg-white/5 px-6 py-5 text-left text-base text-foreground hover:bg-white/10"
>
<a
href="https://github.com/utooland/build-tools-performance"
target="_blank"
rel="noopener noreferrer"
>
<span>{t.benchmark.cta}</span>
<ArrowUpRight className="h-5 w-5 shrink-0" />
</a>
</Button>
| summaryTitle: "Where the leaders stand out", | ||
| summaryDescription: | ||
| "A quick read on the best cold-start, build, and HMR results across the benchmark suite.", |
There was a problem hiding this comment.
There was a problem hiding this comment.
Pull request overview
Adds a new “Benchmark” section to the homepage, backed by new i18n strings, to showcase build tool performance highlights and a per-workload comparison table.
Changes:
- Added a new
Benchmarkhomepage section component with highlights, source info, and a comparison table. - Extended English + Chinese translation dictionaries with benchmark-related copy.
- Added a screenshot asset for the PR description/docs.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/i18n/translations.ts | Adds benchmark translation strings for EN/ZH. |
| app/components/Benchmark.tsx | New client component rendering the benchmark section UI/table. |
| app/(home)/page.tsx | Inserts the new <Benchmark /> section into the homepage flow. |
| .github/pr-assets/benchmark-desktop.jpg | Adds screenshot asset for PR documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| size="lg" | ||
| variant="outline" | ||
| className="h-auto w-full justify-between rounded-[1.75rem] border-white/10 bg-white/5 px-6 py-5 text-left text-base text-foreground hover:bg-white/10" | ||
| onClick={() => | ||
| window.open( | ||
| "https://github.com/utooland/build-tools-performance", | ||
| "_blank", | ||
| ) | ||
| } | ||
| > | ||
| <span>{t.benchmark.cta}</span> | ||
| <ArrowUpRight className="h-5 w-5 shrink-0" /> |
There was a problem hiding this comment.
The CTA uses window.open with target "_blank" but without disabling access to window.opener, which enables reverse-tabnabbing. Prefer rendering the Button as an anchor (Button asChild + ) with rel="noopener noreferrer", or pass a third window.open feature string including "noopener,noreferrer".
| size="lg" | |
| variant="outline" | |
| className="h-auto w-full justify-between rounded-[1.75rem] border-white/10 bg-white/5 px-6 py-5 text-left text-base text-foreground hover:bg-white/10" | |
| onClick={() => | |
| window.open( | |
| "https://github.com/utooland/build-tools-performance", | |
| "_blank", | |
| ) | |
| } | |
| > | |
| <span>{t.benchmark.cta}</span> | |
| <ArrowUpRight className="h-5 w-5 shrink-0" /> | |
| asChild | |
| size="lg" | |
| variant="outline" | |
| className="h-auto w-full rounded-[1.75rem] border-white/10 bg-white/5 text-base text-foreground hover:bg-white/10" | |
| > | |
| <a | |
| href="https://github.com/utooland/build-tools-performance" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="flex w-full items-center justify-between px-6 py-5 text-left" | |
| > | |
| <span>{t.benchmark.cta}</span> | |
| <ArrowUpRight className="h-5 w-5 shrink-0" /> | |
| </a> |
| caseName: "react-1k", | ||
| modules: "1,000 components", | ||
| startup: { tool: "Rsbuild", value: "1529ms" }, | ||
| build: { tool: "Vite (Rolldown)", value: "628ms" }, | ||
| hmr: { tool: "Rspack CLI", value: "107ms" }, | ||
| }, | ||
| { | ||
| caseName: "react-5k", | ||
| modules: "5,000 components", | ||
| startup: { tool: "Rsbuild", value: "1346ms" }, | ||
| build: { tool: "Vite (Rolldown)", value: "1921ms" }, | ||
| hmr: { tool: "Farm", value: "85ms" }, | ||
| }, | ||
| { | ||
| caseName: "react-10k", | ||
| modules: "10,000 components", | ||
| startup: { tool: "Rsbuild", value: "1188ms" }, |
There was a problem hiding this comment.
The per-case "modules" label is hardcoded in English (e.g., "1,000 components"), so the Chinese homepage will still show English copy. If the goal is fully localized benchmark content, consider moving this label/unit into i18n (or formatting the count with Intl.NumberFormat and translating the unit).
| <CardTitle className="text-2xl font-bold">{t.benchmark.tableTitle}</CardTitle> | ||
| <p className="text-sm leading-7 text-muted-foreground/75"> | ||
| {t.benchmark.tableDescription} | ||
| </p> | ||
| </CardHeader> | ||
| <CardContent className="p-0"> | ||
| <div className="overflow-x-auto"> | ||
| <table className="w-full min-w-[760px] text-left"> | ||
| <thead className="bg-white/5"> | ||
| <tr className="text-sm uppercase tracking-[0.2em] text-muted-foreground/60"> | ||
| <th className="px-6 py-4 font-medium">{t.benchmark.columns.case}</th> | ||
| <th className="px-6 py-4 font-medium">{t.benchmark.columns.startup}</th> | ||
| <th className="px-6 py-4 font-medium">{t.benchmark.columns.build}</th> | ||
| <th className="px-6 py-4 font-medium">{t.benchmark.columns.hmr}</th> |
There was a problem hiding this comment.
For table accessibility, the header cells should declare scope="col" (and optionally add a or aria-describedby pointing at the CardTitle/description) so screen readers can correctly associate headers with cells.
| <CardTitle className="text-2xl font-bold">{t.benchmark.tableTitle}</CardTitle> | |
| <p className="text-sm leading-7 text-muted-foreground/75"> | |
| {t.benchmark.tableDescription} | |
| </p> | |
| </CardHeader> | |
| <CardContent className="p-0"> | |
| <div className="overflow-x-auto"> | |
| <table className="w-full min-w-[760px] text-left"> | |
| <thead className="bg-white/5"> | |
| <tr className="text-sm uppercase tracking-[0.2em] text-muted-foreground/60"> | |
| <th className="px-6 py-4 font-medium">{t.benchmark.columns.case}</th> | |
| <th className="px-6 py-4 font-medium">{t.benchmark.columns.startup}</th> | |
| <th className="px-6 py-4 font-medium">{t.benchmark.columns.build}</th> | |
| <th className="px-6 py-4 font-medium">{t.benchmark.columns.hmr}</th> | |
| <CardTitle id="benchmark-table-title" className="text-2xl font-bold"> | |
| {t.benchmark.tableTitle} | |
| </CardTitle> | |
| <p | |
| id="benchmark-table-description" | |
| className="text-sm leading-7 text-muted-foreground/75" | |
| > | |
| {t.benchmark.tableDescription} | |
| </p> | |
| </CardHeader> | |
| <CardContent className="p-0"> | |
| <div className="overflow-x-auto"> | |
| <table | |
| aria-labelledby="benchmark-table-title" | |
| aria-describedby="benchmark-table-description" | |
| className="w-full min-w-[760px] text-left" | |
| > | |
| <thead className="bg-white/5"> | |
| <tr className="text-sm uppercase tracking-[0.2em] text-muted-foreground/60"> | |
| <th scope="col" className="px-6 py-4 font-medium"> | |
| {t.benchmark.columns.case} | |
| </th> | |
| <th scope="col" className="px-6 py-4 font-medium"> | |
| {t.benchmark.columns.startup} | |
| </th> | |
| <th scope="col" className="px-6 py-4 font-medium"> | |
| {t.benchmark.columns.build} | |
| </th> | |
| <th scope="col" className="px-6 py-4 font-medium"> | |
| {t.benchmark.columns.hmr} | |
| </th> |
| summaryTitle: "Where the leaders stand out", | ||
| summaryDescription: | ||
| "A quick read on the best cold-start, build, and HMR results across the benchmark suite.", |
There was a problem hiding this comment.
The new benchmark translations include summaryTitle/summaryDescription, but they don't appear to be used by the Benchmark component. Either render these strings in the section UI or remove them to avoid unused translation surface area.
| summaryTitle: "Where the leaders stand out", | |
| summaryDescription: | |
| "A quick read on the best cold-start, build, and HMR results across the benchmark suite.", |
| summaryTitle: "领先结果一眼看清", | ||
| summaryDescription: | ||
| "快速展示 benchmark 套件里冷启动、生产构建和 HMR 的最佳成绩。", |
There was a problem hiding this comment.
The zh benchmark translations include summaryTitle/summaryDescription, but the Benchmark component doesn't appear to render them. Either wire them into the UI or remove them to avoid carrying unused translation keys.
| summaryTitle: "领先结果一眼看清", | |
| summaryDescription: | |
| "快速展示 benchmark 套件里冷启动、生产构建和 HMR 的最佳成绩。", |
9e1152c to
9cd96be
Compare
Summary
utooland/build-tools-performanceVerification
npm run buildScreenshot