Rebuild website on Primer Brand design prototype - #2703
Open
aaronpowell wants to merge 37 commits into
Open
Conversation
Replace the Astro + Starlight site with the Brand Engineering design prototype, ported component-for-component onto plain Astro + React islands. The prototype is treated as the authority on markup and styling; dynamic data is injected into its components rather than the components being reinterpreted. Framework: - Remove @astrojs/starlight entirely, along with its document shell, search, footer and language selector. BaseLayout.astro now owns the document head, CSP, social meta and analytics. - Add @primer/react-brand and @astrojs/react. Alias the package to its ESM build in vite.resolve, since the default CJS entrypoint breaks named-export detection during SSR, and mark it noExternal so its stylesheet imports resolve. - Promote pagefind to an explicit devDependency; it was previously pulled in transitively by Starlight. Pages: home, the five catalogs, the five detail routes, contributors, Playbook index and articles, and the cookbook are all rendered by ported prototype components inside a shared PageShell. Detail pages share a DetailChassis (hero, breadcrumbs, sticky TOC with scroll-spy, prev/next) while keeping what makes each type distinct: a file switcher over bundled skill assets, an included-items grid and external provenance for plugins, and a hero-scale preview for extensions. i18n: resolve translated Playbook entries via Astro.currentLocale inside the shared article route instead of separate [locale] routes. The explicit routes collided with the i18n fallback routes, so translated articles were being shadowed by their English originals and 900 nonsensical double-locale pages were emitted. Search: TopNavSearch now queries the Pagefind index client-side in addition to the static resource index, merging on href and degrading to the static index in dev, where no index has been built. Also delete the superseded vanilla-TS page renderers and Astro partials, which the ported React components fully replace. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Fixes the three axe violations surfaced by the a11y audit after the Primer Brand redesign: - aria-prohibited-attr: `aria-label` was set on roleless `div`s in InstructionDetail and PluginDetail. The "Applies to" list now uses list/listitem roles; plugin provenance uses a group role. - scrollable-region-focusable: the install command `code` element overflows horizontally but was not keyboard reachable. Added tabIndex to all three render sites. - color-contrast: the Playbook "New" label used the brand's success-fg on success-subtle, reaching only 4.09:1 in light mode. Stepped one down the same green ramp for 6.14:1. Scoped to light mode; dark mode already passed and its green-7 is near-black. Also corrects the stale route list in the audit script: /hooks/, /workflows/ and /tools/ have never existed as pages. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Two issues surfaced by review of the redesigned site: Catalog facets rendered every option. The prototype's filter groups were built from small hardcoded arrays, but real data produces 193 tool options on /agents/ and 245 "Applies to" values on /instructions/. The sidebar grew to ~10,000px and stretched the whole catalog row, pushing the (already present) pagination control far below the fold so it read as missing. Adopt the prototype's own solution for this, which it had already applied to the extensions page: collapse groups past 10 options behind a "Show N more" toggle, and cap .filterOptions with an internal scroll area. Ported verbatim to the agents, instructions, skills, and plugins catalogs. Detail pages scroll inside .scrollHost rather than the document, but the footer came from PageShell, outside that element, so it stayed pinned over the content instead of appearing at the end. The prototype renders its footer inside the scroll host; PageShell now takes a renderFooter flag so DetailChassis can do the same. LearningArticleLayout already did this. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Injected markdown inherited color from body, which sits outside the ThemeProvider and always resolved the light-mode token, making body copy unreadable in dark mode. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Resource detail pages (agent, instruction, skill, plugin, extension) are not articles, and their markdown headings do not form a meaningful outline. Playbook and cookbook articles keep their TOC. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Raw markdown-injected <a> elements previously fell back to the browser's default blue/purple link colours, which are harsh against the dark-mode background. Route them through --brand-color-text-link-rest (and the pressed/hover token) instead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
…mode works LearningArticleLayout replaces PageShell for Playbook articles but never wrapped itself in a ThemeProvider, so its useTheme() call always fell back to Primer's light default regardless of the site's actual theme preference. Split the component into a thin ThemeProvider wrapper plus the existing implementation (now LearningArticleLayoutBody), matching the pattern already used by PageShell. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Astro's default Shiki config bakes in the fixed 'github-dark' theme's literal colors, ignoring the site's actual light/dark mode - this made plain markdown-fenced code blocks (as opposed to the prototype's own SyntaxHighlightedCode component) always render a hardcoded dark box regardless of theme. Switch shikiConfig to the 'css-variables' theme so highlighted tokens resolve through --astro-code-* custom properties instead, then map those to the same brand color tokens the prototype's codeBlock uses (canvas-subtle background, border-muted border, brand text/link/accent colors for tokens). Raw markdown code blocks now match the prototype's bordered, canvas-subtle surface in both color modes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
The contributor badge rendered 0 on Playbook, Cookbook, home and custom pages, and reverted to 0 on hydration everywhere else. Two causes: - Shells that bypass PageShell (LearningArticleLayout, PlaybookIndex, PlaybookArticleBody, CookbookIndex, HomePage, TopNav, Custom) defaulted contributorsTotal to 0 instead of the site-data value. - site-data read .all-contributorsrc with node:fs at module scope. Those shells are client:load hydrated, so the read threw in the browser and the count reset to 0 after hydration. The count is now read once in astro.config.mjs and inlined through vite.define as __CONTRIBUTORS_TOTAL__, so it is a literal in both the server render and the client bundle. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
The filter option lists only set scrollbar-width: thin, so they rendered the platform default scrollbar. They now use the same muted, transparent track treatment as the article sidebar, which resolves through --brand-color-border-muted in both colour modes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Plugin detail pages exposed only a copyable CLI command. They now lead with a ghapp://plugins/install deep link in the same split-button ActionMenu the other detail pages use, keeping the CLI command available as a Copy action in the menu. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Astro 7 deprecates markdown.remarkPlugins in favour of passing a unified() processor from @astrojs/markdown-remark. Moves the GitHub admonitions plugin into markdown.processor, clearing the startup deprecation warning. shikiConfig stays at the markdown level as it is not part of UnifiedProcessorOptions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Silently falling back to 0 is how the contributor badge regressed before, so a missing or malformed .all-contributorsrc now throws in production builds and warns in dev instead of shipping a wrong count. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
github-actions
Bot
added
new-submission
Contributor
|
🟡 Contributor Reputation Check: MEDIUM risk
Maintainers: please review this contributor before merging. |
github-actions
Bot
added
the
needs-review:MEDIUM
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Fix real typos flagged by codespell: 'Couldn't' -> 'Couldn't' (plain apostrophe, matching convention elsewhere in JSX) and 'Unparseable' -> 'Unparsable' in catalogFilters.ts - DetailChassis.tsx: replace sequential HTML entity unescaping with a single-pass replace to avoid double-unescape/injection risk flagged by CodeQL - SyntaxHighlightedCode.tsx: make the markup HTML comment regex match newlines so multi-line comments cannot break out of the token (Bad HTML filtering regexp) - pagefindSearch.ts: strip HTML tags in a loop until stable so nested/ malformed markup can't survive a single-pass strip (Incomplete multi-character sanitization) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (14)
Previously missed (3) — in code that hasn't changed since the last review.
website/src/components/brand/ExtensionsCatalog.tsx:384
- Generated in-repo extension records use
installUrlfor the GitHub source tree, not a Copilot deep link (eng/generate-website-data.mjs:1234-1260). Consequently, this “Open in Copilot app” button opens GitHub. Build theghapp://plugins/install?source=...URL frompluginName, as the detail route does.
website/src/components/brand/AgentsCatalog.tsx:151 - The generated model facet includes
(none), but agents without a model produce an emptymodelsarray, so selecting(none)always returns zero matches. Treat that sentinel asmodels.length === 0.
website/src/components/brand/PageShell.tsx:59 - Because this provider is created inside
PageShell,useTheme()calls in parent components such asAgentsCatalog,InstructionsCatalog, andPluginDetailexecute outside it. Theirdata-mode={colorMode}values therefore cannot receive the resolved dark mode, so the dark-only grid/menu overrides do not activate. Hoist the provider above the hydrated page component or move those hooks into descendants rendered under this provider.
website/src/components/brand/SkillsCatalog.tsx:135
skill.filesis generated as an array of file records, but this component types it as a number and passes the array tofileBucketOf. Numeric comparisons then produce no match and the fallback classifies multi-file skills as “1–2 files.” Typefilesas the generated array shape and passskill.files.length.
website/src/components/brand/SkillsCatalog.tsx:49- This URL uses GitHub’s
/blob/route for a directory, so the catalog’s source button leads to a 404. Skill folders need the/tree/route (or a/blob/link toSKILL.md).
website/src/components/brand/searchIndex.ts:120 staticPagesis defined for the fallback index but never added here. When Pagefind is unavailable (notably underastro dev, which is an explicitly supported fallback), searches for Home, Playbook, Contributors, or catalog landing pages return no static result.
website/src/components/brand/navigation.ts:17- The rebuild drops the existing Hooks, Workflows, and Tools destinations (and their routes are absent), so established
/hooks/,/workflows/,/tools/, and detail URLs now 404. This conflicts with the PR note that pages without a prototype were retained using the existing UX. Restore those resource routes/navigation or explicitly treat this as a breaking scope change.
website/src/components/brand/SkillsCatalog.tsx:125 - The success state and “Install command copied!” announcement are set even when Clipboard API access throws. Only set
copiedafterwriteTextresolves, otherwise users are told a command was copied when their clipboard is unchanged.
website/src/components/brand/ExtensionsCatalog.tsx:98 - This marks the action as copied and triggers the success toast even when Clipboard API support is absent or
writeTextrejects. Await the write and only callsetCopied/onCopiedafter it succeeds.
website/src/components/brand/PluginsCatalog.tsx:125 - The previous catalog exposed “Recently Updated,” but this state is permanently initialized to
azand its setter is discarded, making thenewestbranch unreachable. This catalog therefore no longer provides the sorting behavior claimed in the PR description.
website/src/components/brand/InstructionsCatalog.tsx:114 - The sort mode can never change because the state setter is discarded and no sort control is rendered, so the
newestpath is unreachable. This removes the prior “Recently Updated” sorting behavior despite the PR description stating catalogs support sorting.
website/src/components/brand/AgentsCatalog.tsx:160 - This hardcodes alphabetical ordering and the new catalog has no sort state or control, removing the previous “Recently Updated” option. That contradicts the stated catalog sorting scope.
website/src/components/brand/ExtensionsCatalog.tsx:193 - This hardcodes alphabetical ordering and no sort control is rendered, so the previous “Recently Updated” behavior is lost. That contradicts the PR’s claim that catalog pages support sorting.
website/src/components/brand/SkillsCatalog.tsx:141 - This hardcodes alphabetical ordering and no sort control is rendered, so the previous “Recently Updated” behavior is lost. That contradicts the PR’s claim that catalog pages support sorting.
- Files reviewed: 73/164 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Most of the site is English-only, but the language selector rendered unconditionally on every page even though only the learning-hub/copilot-workshops/app track has mirrored translations. Add hasTranslations() to playbook-routes.ts, thread a showLanguageSelect prop through PageShell, TopNav and LearningArticleLayout, and compute it from the article's englishId in learning-hub/[...slug].astro so only translated Playbook articles show the selector. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (15)
Previously missed (3) — in code that hasn't changed since the last review.
website/src/components/brand/AgentsCatalog.tsx:428
- Changing pages only swaps the six cards while the viewport remains at the pagination control below the grid, so users do not see the newly selected page. Scroll the catalog back into view after updating the page (and respect reduced-motion preferences).
website/src/components/brand/SkillsCatalog.tsx:355 - Changing pages only swaps the six cards while the viewport remains at the pagination control below the grid, so users do not see the newly selected page. Scroll the catalog back into view after updating the page (and respect reduced-motion preferences).
website/src/components/brand/InstructionsCatalog.tsx:565 - Changing pages only swaps the six cards while the viewport remains at the pagination control below the grid, so users do not see the newly selected page. Scroll the catalog back into view after updating the page (and respect reduced-motion preferences).
website/src/components/brand/SkillsCatalog.tsx:49
- This URL points a directory at GitHub's
/blob/route, which returns a missing-file page. Skills are folders; use/tree/main/skills/...as the detail page already does.
website/src/components/brand/SkillsCatalog.tsx:135 skills.jsonemitsfilesas an array of file records, but this component declares it as a number and passes the array directly tofileBucketOf. At runtime arrays coerce toNaN(except an empty array), so every populated skill falls back to “1–2 files” and the 3–5/6–10/11+ filters return incorrect results. Typefilesas the generated file array and bucketskill.files.length.
website/src/components/brand/searchIndex.ts:120staticPages()is never added to the returned index, despite the adapter promising fixed top-level destinations. When Pagefind is unavailable (includingastro dev), searches for Home, Playbook, Contributors, Extensions, or any catalog landing page therefore return no destination. Seed the result list withstaticPages(base).
website/scripts/a11y-audit.mjs:35- Removing the only concrete Playbook article route means this audit now exercises the Playbook index and Cookbook index, but never the new
PlaybookArticleBodyarticle chassis. Keep a representative article such as/learning-hub/github-copilot-app/so the claimed article accessibility coverage remains enforced.
website/src/components/brand/PluginsCatalog.tsx:381 - Changing pages only swaps the six cards while the viewport remains at the pagination control below the grid, so users do not see the newly selected page. Scroll the catalog back into view after updating the page (and respect reduced-motion preferences).
website/src/components/brand/ExtensionsCatalog.tsx:444 - Changing pages only swaps the six cards while the viewport remains at the pagination control below the grid, so users do not see the newly selected page. Scroll the catalog back into view after updating the page (and respect reduced-motion preferences).
website/src/components/brand/PluginsCatalog.tsx:125 sortModecan never change because its setter is discarded and there is no sort control, making the declarednewestmode unreachable. This contradicts the PR's catalog-sorting scope and removes the previous “Recently Updated” option; wire a selector tosetSortModeand sort bylastUpdatedfornewest.
website/src/components/brand/InstructionsCatalog.tsx:114sortModecan never change because its setter is discarded and there is no sort control, so the existingnewestbranch is unreachable. This contradicts the PR's catalog-sorting scope and removes the previous “Recently Updated” option; wire a selector tosetSortMode.
website/src/components/brand/SkillsCatalog.tsx:141- The PR describes sorting on every catalog, but this catalog always applies title order and exposes no sort state or control, so “recently updated” sorting from the previous page is lost. Add a sort selector and switch this comparator between title and
lastUpdated.
website/src/components/brand/ExtensionsCatalog.tsx:193 - The PR describes sorting on every catalog, but this catalog always applies name order and exposes no sort state or control, so “recently updated” sorting from the previous page is lost. Add a sort selector and switch this comparator between name and
lastUpdated.
website/src/components/brand/AgentsCatalog.tsx:160 - The PR describes sorting on every catalog, but this catalog always applies title order and exposes no sort state or control, so “recently updated” sorting from the previous page is lost. Add a sort selector and switch this comparator between title and
lastUpdated.
website/src/components/brand/ExtensionsCatalog.tsx:380 installUrlcomes from external extension metadata and is rendered directly into an anchor. Unlike the previous renderer and the detail route, this path does not run it throughsanitizeHttpUrl, so a malformedjavascript:/data:value can become an executable catalog action. Sanitize externalinstallUrl/sourceUrlvalues before rendering and omit the button when sanitization fails.
- Files reviewed: 73/164 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The copilot-workshops hands-on tracks (VS Code, CLI, App, Cloud harnesses) existed and rendered correctly at direct URLs, but had no entry point in the new Playbook index UI -- the Articles grid intentionally excludes nested multi-page tracks by design, and the only prior link was buried in body prose. Adds a 'Workshop' recommended card (following the existing cli-for-beginners precedent) linking to /learning-hub/copilot-workshops/, and registers the route in pageHref.ts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (25)
Previously missed (3) — in code that hasn't changed since the last review.
website/src/components/brand/InstructionsCatalog.tsx:565
- Changing pages leaves the viewport at the bottom pagination control after the card grid is replaced, so the newly selected page appears above the user. Scroll
#cataloginto view after the state update and respect reduced-motion preferences.
website/src/components/brand/SkillsCatalog.tsx:355 - Changing pages leaves the viewport beside the pagination control while the next six cards render above it. This recreates the catalog pagination/scrolling problem called out in the PR description; scroll the catalog into view after changing pages.
website/src/components/brand/AgentsCatalog.tsx:151 - The generated model options explicitly include the
"(none)"sentinel, butmodels.includes("(none)")can never match an agent whose model list is empty. Selecting that advertised filter therefore returns zero results; handle the sentinel by matchingmodels.length === 0.
This issue also appears on line 425 of the same file.
website/src/components/brand/searchIndex.ts:132
staticPages()is never added to the returned index, so when Pagefind is unavailable (includingastro dev) searches for Home, Playbook, or Contributors return no result despite the documented static fallback. Include the fixed destinations in the index.
website/src/components/brand/AgentsCatalog.tsx:160- This always alphabetizes the catalog; there is no sort state or control for the previously available “Recently Updated” ordering. That regresses the advertised sorting behavior, so expose a sort selector and apply
daysSince(lastUpdated)when selected.
website/src/components/brand/InstructionsCatalog.tsx:114 sortModehas no setter and is permanently"az", making the existing"newest"branch unreachable and leaving users without the advertised sort control. Add a selector that updates this state.
website/src/components/brand/ExtensionsCatalog.tsx:193- Extensions are unconditionally alphabetized, with no sort state or control for recently updated items. This leaves this catalog without the sorting behavior promised by the PR; add a selector and apply a last-updated comparator.
website/src/components/brand/SkillsCatalog.tsx:141 - The rebuilt skills catalog always sorts alphabetically and provides no recently-updated ordering, although sorting is part of the stated catalog functionality and existed before this migration. Add sort state/control and use
daysSincefor the updated order.
website/src/components/brand/PluginsCatalog.tsx:125 sortModecan never change because its setter is discarded. Moreover, the non-azpath does not sort bylastUpdated, so even wiring a control would produce generator order rather than “newest”; implement the comparator and selector together.
website/scripts/a11y-audit.mjs:32- The rebuilt router no longer contains
/hooks/,/workflows/, or/tools/(nor their detail routes), so removing them from this audit hides user-facing 404 regressions for existing resource types. Restore those routes and keep representative audit coverage rather than dropping the checks.
website/src/components/brand/AgentsCatalog.tsx:428 - Changing pages only swaps the six rendered cards while leaving the viewport at the pagination control below them, so users do not see the new page without manually scrolling back up. Scroll the catalog into view after updating the page (while respecting reduced motion).
website/src/components/brand/PluginsCatalog.tsx:381 - The page transition only updates state, so the viewport remains below the newly rendered plugin cards at the pagination control. Scroll the catalog back into view after selecting a page, using instant scrolling for reduced-motion users.
website/src/components/brand/ExtensionsCatalog.tsx:444 - After pagination, the viewport remains at the control below the grid while the selected extension page renders above it. Scroll
#cataloginto view aftersetCurrentPageso the pagination/scrolling fix is effective.
website/src/components/brand/AgentDetail.tsx:98 downloadUrlpoints atraw.githubusercontent.com, so the HTMLdownloadattribute is ignored by browsers for this cross-origin URL and this menu item navigates to the raw file instead of downloading it. Fetch the file and download a same-origin Blob (or serve a same-origin download endpoint); the icon action below has the same issue.
website/src/components/brand/InstructionDetail.tsx:90- Because this URL is on
raw.githubusercontent.com, browsers ignore the cross-origindownloadattribute and navigate to the raw text rather than saving a file. Use a fetched Blob/same-origin URL for both this item and the duplicate icon action below.
website/src/components/brand/SkillsCatalog.tsx:311 - The raw GitHub URL is cross-origin, so browsers ignore this anchor's
downloadattribute and openSKILL.mdrather than saving it. Implement the download through a fetched Blob/same-origin object URL.
website/src/components/brand/AgentsCatalog.tsx:384 - The
downloadattribute does not force downloads for this cross-originraw.githubusercontent.comlink, so the card action opens the raw file instead of downloading it. Fetch the content into a Blob and trigger a same-origin object URL, as the existing file-browser flows do.
website/src/components/brand/InstructionsCatalog.tsx:521 - This card links cross-origin to
raw.githubusercontent.com; browsers ignoredownloadin that case, so clicking the advertised download action navigates away instead of saving the instruction. Use a fetched Blob or same-origin endpoint.
website/src/components/brand/SkillsCatalog.tsx:45 - The generated skills data stores
filesas an array of file records, not a number (generate-website-data.mjsassignsfilesdirectly). Because the page casts data toany, this mismatch reachesfileBucketOf(skill.files), where array-to-number comparisons fail and every skill falls back to “1–2 files.” Type this as an array and bucketskill.files.length.
website/src/components/brand/SkillsCatalog.tsx:121 - The success toast is shown even when
clipboard.writeTextrejects, because the catch is ignored andsetCopied(true)runs unconditionally. Only enter the copied state after a successful write so the UI does not falsely report success.
website/src/components/brand/ExtensionsCatalog.tsx:94 - This reports “Copied” and fires the success toast even when the Clipboard API is unavailable or the write rejects. Await the write and update success state only after it resolves.
website/src/components/brand/ExtensionDetail.tsx:67 - The button switches to “Copied” even if the Clipboard API is missing or rejects the write. Make the handler async and update the success state only after
writeTextresolves.
website/src/components/brand/SkillsCatalog.tsx:49 - This is a directory URL, but it uses GitHub's
/blob/route, so the card's source action does not point to the skill folder. Use/tree/main/skills/...as the detail page already does.
website/src/components/brand/PlaybookArticleBody.tsx:95 - Each section chunk still contains its original
<h2 id="…">, so assigning the same ID to this wrapper produces duplicate IDs for every article heading. Fragment links and scroll tracking can resolve the wrapper instead of the heading; leave the ID on the rendered heading only.
website/scripts/a11y-audit.mjs:35 - The audit now covers only the Playbook landing and Cookbook index; removing
/learning-hub/github-copilot-app/means no actualPlaybookArticleBody/LearningArticleLayoutarticle is scanned. Keep a representative article route so the article-specific accessibility claim remains enforced.
- Files reviewed: 73/164 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The hands-on workshop tracks (CLI for Beginners, and the copilot-workshops harnesses) had no shared way to discover them from the Articles grid -- each had a recommended card, but no consistent categorization. Tags both workshop landing pages 'workshop', adds a new 'Workshop' Kind/content-type facet (derived the same way as the existing Terminology/Tutorial/Example kinds), and includes both landing pages in the Articles grid dataset so filtering by Content type > Workshop surfaces exactly the two tracks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
website/src/components/brand/ExtensionsCatalog.tsx:380
installUrlis the generated GitHub tree URL, not aghapp://deep link (the detail route explicitly converts it atpages/extension/[id].astro:111-121). This button therefore opens the repository while announcing “Open in Copilot app.” Build the app URL frompluginNamehere as the detail route does, and keepinstallUrlfor the source/fallback action.
website/src/components/brand/PlaybookArticleBody.tsx:81- This newly adopted article chassis stops receiving scroll updates below 75rem: its stylesheet changes
.scrollHosttooverflow: visible(github-copilot-app.module.css:1896-1956), whileLearningArticleLayoutonly reads/listens to that element’sscrollTop. Playbook and Cookbook pages at those widths therefore never update reading progress or expose the back-to-top control. Update the chassis to branch to window scroll metrics/listeners, as the detail-page hook already does.
website/src/components/brand/SkillsCatalog.tsx:135
- Generated skill records contain
filesas an array (generate-website-data.mjs:373-402), but this component types it as a number and passes the array directly into numeric bucket comparisons. Non-empty object arrays coerce toNaN, so every skill falls back to “1–2 files” and the other bundled-file filters return no results. Bucket on the array length (and update the item type accordingly).
website/src/components/brand/searchIndex.ts:120 - The fixed top-level destinations are defined by
staticPages(base)but never added to the returned index. Consequently, when Pagefind is unavailable (includingastro dev, the documented fallback case), searches for Home, Playbook, or Contributors cannot return those pages. Seed the result array with the static destinations.
- Files reviewed: 73/166 changed files
- Comments generated: 0 new
- Review effort level: Balanced
- P0: html lang now reflects the actually-rendered locale for a page, not the requested URL locale (BaseLayout.astro, playbook-routes.ts, PlaybookArticle.astro) - P0: markdown tables get tabindex=0 via enhanceMarkdownA11y, which Playbook articles were never running (PlaybookArticle.astro) - P0: long inline content and breadcrumbs no longer clip on narrow viewports (dotnet-upgrade.module.css, github-copilot-app.module.css) - P1: strip the markdown document's own leading H1 so detail pages don't render the title twice (detail-page.ts) - P1: document the hero secondary-CTA hex and InstructionsCatalog illustration colors as intentional prototype-fidelity values rather than defects (styles.module.css) - P1: add explicit Heading size props in PlaybookIndex.tsx Ref: github/brand-experience#458
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (2)
website/src/components/brand/PlaybookArticleBody.tsx:95
- Each chunk still contains its leading
<h2 id="…">, so assigning the same ID to this outer section creates duplicate IDs for every Playbook heading. Remove the wrapper ID (the existing heading remains the TOC/scroll target) or strip the ID from the nested heading.
website/src/components/brand/searchIndex.ts:132 staticPagesis defined specifically so main destinations are always searchable, but it is never included in the returned index. Because Pagefind is unavailable inastro dev, searches for Home, Playbook, Contributors, or catalog landing pages then return no fallback result. Prepend the static destinations to the generated records.
- Files reviewed: 73/167 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Renames PlaybookIndex.tsx -> LearningHubIndex.tsx, PlaybookArticle.astro -> LearningHubArticle.astro, PlaybookArticleBody.tsx -> LearningHubArticleBody.tsx, PlaybookIcon.tsx -> LearningHubIcon.tsx, lib/playbook-routes.ts -> lib/learning-hub-routes.ts, and lib/playbook-article.ts -> lib/learning-hub-article.ts. Updates all call sites, UI copy, TopNav's playbookLabel prop, nav/search entries, and home page CTA data to use Learning Hub terminology. URLs under /learning-hub/* are unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Adopts the prototype's github-copilot-app.tsx directly as a dedicated React component instead of rendering it through the generic markdown pipeline, preserving its video carousel, comparison list, learn-more band, and other bespoke JSX exactly as designed. - New GithubCopilotApp.tsx under components/brand/learning-hub/, ported near-verbatim from the prototype (data-wiring only: pageHref import swap, media paths). - The two large source videos (~23MB, ~32MB) are not committed; the article falls back to their poster/webp images with a TODO to host them externally. - learning-hub-bespoke-articles.ts lists slugs that bypass the generic LearningHubArticle.astro pipeline. - [...slug].astro branches to the bespoke component for "github-copilot-app" (English only for now); all other slugs and locales keep the generic path.
Add bespoke Learning Hub article components for the remaining prototype-backed article pages, wire the cookbook index to the data-driven port, and route the CLI overview page through the new bespoke component. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80686fef-efe3-4cdd-8cd6-bfa61a5d0af6
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
website/src/components/brand/searchIndex.ts:120
staticPages()is never added to the returned index, despite being defined specifically for the top-level destinations. In development, where Pagefind is intentionally unavailable, searches for “Home”, “Contributors”, or a catalog landing page therefore return no page result. Seed the result array with these entries.
- Files reviewed: 78/181 changed files
- Comments generated: 6
- Review effort level: Balanced
| id: entry.id, | ||
| title: entry.data.title, | ||
| description: entry.data.description ?? "", | ||
| href: `/${entry.id}/`, |
| // Scrollable <pre>/<table> blocks in this markdown need to be made keyboard | ||
| // focusable the same way detail-page markdown already is (see | ||
| // lib/detail-page.ts) — Learning Hub articles were missing this pass entirely. | ||
| const html = enhanceMarkdownA11y(entry.rendered?.html ?? ""); |
| const destinations = [ | ||
| { label: "Agents", page: "agents" }, | ||
| { label: "Instructions", page: "instructions" }, | ||
| { label: "Learning Hub", page: "learning-hub-copilot-app" }, |
Comment on lines
+132
to
+136
| if (window.matchMedia(TWO_COLUMN_QUERY).matches) { | ||
| scrollHost.scrollTo({ top: 0, behavior: "smooth" }); | ||
| } else { | ||
| window.scrollTo({ top: 0, behavior: "smooth" }); | ||
| } |
Comment on lines
+194
to
+196
| const scrollToTop = () => { | ||
| contentScrollRef.current?.scrollTo({ top: 0, behavior: "smooth" }); | ||
| }; |
Comment on lines
+43
to
+46
| export function CookbookIndex({ | ||
| sections, | ||
| searchIndex = [], | ||
| contributorsTotal = siteContributorsTotal, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
npm startand verified thatREADME.mdis up to date.mainbranch for this pull request.Description
This rebuilds the
website/app on the Brand Engineering design prototype, replacing the previous Starlight-based UI with components and styling ported directly from the prototype (Astro 7 + React +@primer/react-brand). The prototype is treated as the design authority: catalogs, detail pages, the home page, navigation, search, and the renamed "Playbook" (formerly Learning Hub) all now use the prototype's components as-is, with dynamic data (agent/skill/plugin/contributor counts, search index, catalog listings) injected at build time rather than reinvented.Scope covered by this PR:
ThemeProvideron the Playbook layout), softened scrollbars on filter/sidebar lists, a broken contributor count that reverted to zero after hydration, and an Astro 7 markdown API deprecation warningNotable implementation details
.all-contributorsrcat build time and inlined viavite.define, since several page shells areclient:loadhydrated and can't safely read the filesystem in the browser. A missing/malformed manifest now fails production builds instead of silently rendering0.markdown.remarkPluginsoption to amarkdown.processor: unified({...})config per Astro 7's guidance, preserving GFM, SmartyPants, and the GitHub-admonitions-to-directives conversion used by mirrored Playbook content.ghapp://plugins/install?source=<id>@awesome-copilotdeep link into the Copilot app (mirroring the existing VS Code deep-link pattern on agent pages), with "copy CLI install command" available as a secondary action in the same split-button menu.Type of Contribution
Additional Notes
This PR does not yet cover every page that exists on the current site; pages without a corresponding design prototype were built following the existing site's UX as a functional guide, using the prototype's components as the visual/interaction authority.
npm run buildpasses (1084 pages) andnpm run plugin:validate/npm run skill:validateare unaffected, as this PR only toucheswebsite/.By submitting this pull request, I confirm that my contribution abides by the Code of Conduct and will be licensed under the MIT License.