feat(website): Phosphor icon system + mobile nav fix#843
Conversation
- Add Icon.astro component that reads Phosphor SVG files at build time via fs.readFileSync — zero runtime cost, pure SSG output - Install @phosphor-icons/core for SVG assets - Replace all emoji icons (🤖📖💬✨🛡️⚙️🧩🔧🗺️✅📚🍳☀️🌙) with named Phosphor icons: robot, book-open, chat-text, sparkle, shield-check, gear, puzzle-piece, wrench, compass, shield-star, check-circle, books, cooking-pot, sun, moon - Fix mobile nav menu: add z-index:99 so menu renders above page content - Fix mobile nav scroll-lock: add/remove document.body.overflow=hidden when menu opens/closes (matches homepage nav behaviour) https://claude.ai/code/session_01VaY86RbnELdWvFyBdEMwLH
|
Warning Review limit reached
More reviews will be available in 20 minutes and 28 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR swaps emoji icons for Phosphor SVG icons site-wide via a new build-time ChangesIcon System Migration
Sequence Diagram(s)(Diagram provided in the hidden review stack artifact above.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The PR involves moderate complexity across multiple files with consistent patterns: Icon component implementation logic, dependency management, straightforward page-level icon swaps, and CSS alignment updates. The mobile menu scroll-lock refactor adds a touch of extra logic, but the overall changes follow a repeatable pattern throughout the site. Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Template check passed after update. Thanks for fixing the PR description. |
🔍 Reviewer Summary for PR #843CI Status: ✅ Recommendations
|
There was a problem hiding this comment.
Code Review
This pull request replaces emoji-based icons with SVG icons from the @phosphor-icons/core package across the website, introducing a reusable Icon.astro component to dynamically load and render the SVGs. It also refactors the mobile navigation menu in AwesomeGithubNav.astro to manage body overflow and centralize menu-closing logic. Feedback on these changes suggests using import.meta.resolve in Icon.astro to dynamically resolve package paths instead of hardcoding relative paths to node_modules, and moving the closeMenu helper inside the guard block in AwesomeGithubNav.astro to eliminate unsafe non-null assertions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const svgUrl = new URL( | ||
| `../../../node_modules/@phosphor-icons/core/assets/${weight}/${name}.svg`, | ||
| import.meta.url, | ||
| ); | ||
|
|
||
| let svgContent = ""; | ||
| try { | ||
| const raw = readFileSync(fileURLToPath(svgUrl), "utf-8"); |
There was a problem hiding this comment.
Hardcoding the relative path ../../../node_modules to resolve packages is fragile and can break in monorepos, environments with package hoisting, or when using strict package managers like pnpm. Use import.meta.resolve to dynamically and robustly resolve the path to @phosphor-icons/core.
const svgPath = fileURLToPath(
import.meta.resolve("@phosphor-icons/core/assets/" + weight + "/" + name + ".svg")
);
let svgContent = "";
try {
const raw = readFileSync(svgPath, "utf-8");
| function closeMenu() { | ||
| hamburger!.setAttribute("aria-expanded", "false"); | ||
| hamburger!.setAttribute("aria-label", "Open navigation menu"); | ||
| menu!.classList.remove("ag-nav-menu--open"); | ||
| document.body.style.overflow = ""; | ||
| } | ||
|
|
||
| if (hamburger && menu) { |
There was a problem hiding this comment.
Defining closeMenu outside of the if (hamburger && menu) block forces the use of non-null assertions (!), which can lead to runtime errors if the elements are not found. Moving closeMenu inside the guard block eliminates the need for non-null assertions and ensures type safety.
if (hamburger && menu) {
const closeMenu = () => {
hamburger.setAttribute("aria-expanded", "false");
hamburger.setAttribute("aria-label", "Open navigation menu");
menu.classList.remove("ag-nav-menu--open");
document.body.style.overflow = "";
};
- Use import.meta.resolve() in Icon.astro instead of hardcoded ../../../node_modules path (robust across monorepos/hoisting) - Move closeMenu() inside if (hamburger && menu) guard in AwesomeGithubNav.astro to eliminate non-null assertions - Add CHANGELOG.md entries for Phosphor icon system and mobile nav fix https://claude.ai/code/session_01VaY86RbnELdWvFyBdEMwLH
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1789915e06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| hamburger.setAttribute("aria-expanded", "true"); | ||
| hamburger.setAttribute("aria-label", "Close navigation menu"); | ||
| menu.classList.add("ag-nav-menu--open"); | ||
| document.body.style.overflow = "hidden"; |
There was a problem hiding this comment.
If a visitor opens the drawer at ≤768px and then rotates/resizes to desktop, this inline overflow: hidden remains on body while the hamburger is hidden by the media query, so the desktop page can become unscrollable until the user happens to press Escape or follows a nav link. Please add a resize/media-query cleanup path, or only apply the lock while the mobile query still matches.
Useful? React with 👍 / 👎.
Adds a matchMedia change listener so that if the mobile nav is open and the viewport widens past 768 px (e.g. device rotation, browser resize), closeMenu() fires and removes the body overflow lock, preventing the desktop page from becoming unscrollable. Addresses Codex P2 review comment on #843. https://claude.ai/code/session_01VaY86RbnELdWvFyBdEMwLH
There was a problem hiding this comment.
Pull request overview
This PR updates the Awesome GitHub Astro site to use Phosphor SVG icons (built-time inline SVGs) instead of emoji, and applies a couple of mobile navigation usability fixes (stacking context + scroll lock).
Changes:
- Added a new
Icon.astrocomponent that inlines Phosphor SVG assets at build time, and updated multiple pages to use named icons. - Fixed mobile nav overlay behaviour by adding a
z-indexand locking page scroll while the menu is open. - Added
@phosphor-icons/coreas a dependency and documented the change inCHANGELOG.md.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| website/src/pages/learning-hub/index.astro | Replaces track emoji with <Icon /> and updates icon wrapper styling. |
| website/src/pages/learn/index.astro | Replaces track emoji with <Icon /> and updates icon wrapper styling. |
| website/src/pages/learn/[track]/index.astro | Replaces track emoji with <Icon /> and adjusts hero icon styling. |
| website/src/pages/getting-started.astro | Replaces “What next?” card emoji with <Icon /> and updates icon wrapper styling. |
| website/src/pages/cookbook/[slug].astro | Replaces placeholder emoji with <Icon /> and updates icon wrapper styling. |
| website/src/pages/c/[type]/index.astro | Updates resource type icons to render via <Icon />. |
| website/src/lib/resources.ts | Switches resource type icon values from emoji to Phosphor icon names. |
| website/src/components/AwesomeGithub/Icon.astro | New build-time SVG inliner component for Phosphor icons. |
| website/src/components/AwesomeGithub/AwesomeGithubNav.astro | Updates theme toggle icons to SVG and adds mobile menu z-index + scroll lock. |
| website/package.json | Adds @phosphor-icons/core dependency. |
| website/package-lock.json | Locks @phosphor-icons/core dependency. |
| CHANGELOG.md | Documents the icon system and mobile nav fixes. |
Files not reviewed (1)
- website/package-lock.json: Language not supported
| const raw = readFileSync(svgPath, "utf-8"); | ||
| svgContent = raw.replace("<svg ", `<svg width="${size}" height="${size}" `); |
| } catch { | ||
| svgContent = `<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 256 256" width="${size}" height="${size}" fill="currentColor"></svg>`; | ||
| } |
Replace existing width/height attributes in the Phosphor SVG root element instead of prepending new ones (which produced duplicate attributes and unpredictable sizing). Also log a console.warn with the icon name when a file cannot be resolved so typos surface during the build rather than silently rendering an empty SVG. Addresses Copilot review comments on #843. https://claude.ai/code/session_01VaY86RbnELdWvFyBdEMwLH
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
website/src/pages/learn/[track]/index.astro (1)
17-22: ⚡ Quick winConsider extracting shared
trackIconsmapping to avoid duplication.The
trackIconsobject is identical to the one inlearn/index.astro(lines 9-14). If these mappings need to stay in sync, consider extracting them to a shared constant inlib/learn.ts.♻️ Suggested refactor
In
website/src/lib/learn.ts:export const TRACK_ICONS: Record<string, string> = { oriented: "compass", governance: "shield-star", quality: "check-circle", agents: "robot", };Then import in both files:
+import { LEARN_TRACKS, getTrack, TRACK_ICONS } from "../../../lib/learn"; -const trackIcons: Record<string, string> = { - oriented: "compass", - governance: "shield-star", - quality: "check-circle", - agents: "robot", -};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/src/pages/learn/`[track]/index.astro around lines 17 - 22, Extract the duplicated trackIcons mapping into a shared exported constant (e.g., TRACK_ICONS) in lib/learn.ts and replace the local const trackIcons in website/src/pages/learn/[track]/index.astro (and the other file that has the same mapping) with an import of that constant; update any references to use the imported TRACK_ICONS to keep mappings in one place and avoid drift between files.website/src/pages/c/[type]/index.astro (1)
338-340: 💤 Low valueConsider updating
.ag-type-iconstyling for consistency.The other icon containers in this PR migration (
.gs-next-icon,.recipe-placeholder-icon, etc.) were updated to usedisplay: flexwith explicit colour, but.ag-type-iconretains the oldfont-size: 32pxapproach. Since it now renders SVG via the Icon component, consider switching to flex layout for visual consistency.♻️ Suggested styling update
.ag-type-icon { - font-size: 32px; + display: flex; + color: var(--accent); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/src/pages/c/`[type]/index.astro around lines 338 - 340, The .ag-type-icon CSS still uses font-size: 32px but now renders an SVG via the Icon component; change its styling to match the other migrated icon containers by replacing the font-size rule with a flex-based layout and explicit color handling (e.g., use display: flex; align-items: center; justify-content: center; width/height or padding to control size, and set color or fill as appropriate) so that .ag-type-icon behaves like .gs-next-icon and .recipe-placeholder-icon and the SVG Icon inherits the intended color and sizing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 34-35: The changelog entry for "Awesome GitHub Site: Mobile Nav
Menu" currently only includes the PR link ([`#843`]) and is missing the required
issue link; update the [Unreleased] entry in CHANGELOG.md for that item (the
line containing "Awesome GitHub Site: Mobile Nav Menu" and PR `#843`) to include
the corresponding issue reference (e.g., add "and issue [`#XXX`](...)" or append "
([`#XXX`](...))" next to the PR link) so the entry contains both a PR link and an
issue link following the repository's Keep a Changelog format.
- Around line 30-31: Update the CHANGELOG.md Unreleased entry that currently
references PR [`#843`] by adding the corresponding issue link (replace `#XXX` with
the actual issue number) so the line includes both PR and issue links (e.g.,
"([`#843`](...) ; [`#XXX`](...))"); then reflow/wrap the long bullet for the
Icon.astro/Phosphor change into multiple shorter lines under ~120 characters at
natural sentence boundaries while preserving the exact descriptive text and the
PR/issue metadata format.
In `@website/src/components/AwesomeGithub/AwesomeGithubNav.astro`:
- Around line 87-103: The mobile menu can leave document.body.style.overflow set
to "hidden" after a mobile→desktop resize because the hamburger is hidden and
can't be toggled; update the menu logic to (1) save and restore the previous
document.body.style.overflow value when opening/closing (modify closeMenu and
the click handler that sets overflow to "hidden") and (2) add a
resize/breakpoint handler that detects crossing the mobile breakpoint (e.g.,
window.innerWidth >= 768) and force-closes the menu by calling closeMenu(),
removing the ag-nav-menu--open class, and updating hamburger
aria-expanded/aria-label accordingly to ensure the overflow is always restored.
---
Nitpick comments:
In `@website/src/pages/c/`[type]/index.astro:
- Around line 338-340: The .ag-type-icon CSS still uses font-size: 32px but now
renders an SVG via the Icon component; change its styling to match the other
migrated icon containers by replacing the font-size rule with a flex-based
layout and explicit color handling (e.g., use display: flex; align-items:
center; justify-content: center; width/height or padding to control size, and
set color or fill as appropriate) so that .ag-type-icon behaves like
.gs-next-icon and .recipe-placeholder-icon and the SVG Icon inherits the
intended color and sizing.
In `@website/src/pages/learn/`[track]/index.astro:
- Around line 17-22: Extract the duplicated trackIcons mapping into a shared
exported constant (e.g., TRACK_ICONS) in lib/learn.ts and replace the local
const trackIcons in website/src/pages/learn/[track]/index.astro (and the other
file that has the same mapping) with an import of that constant; update any
references to use the imported TRACK_ICONS to keep mappings in one place and
avoid drift between files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2330b256-2a10-4e4e-8a1f-39dbafbd21e7
⛔ Files ignored due to path filters (1)
website/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
CHANGELOG.mdwebsite/package.jsonwebsite/src/components/AwesomeGithub/AwesomeGithubNav.astrowebsite/src/components/AwesomeGithub/Icon.astrowebsite/src/lib/resources.tswebsite/src/pages/c/[type]/index.astrowebsite/src/pages/cookbook/[slug].astrowebsite/src/pages/getting-started.astrowebsite/src/pages/learn/[track]/index.astrowebsite/src/pages/learn/index.astrowebsite/src/pages/learning-hub/index.astro
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: coderabbit-gate
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{php,js,jsx,ts,tsx,css,scss,html}
📄 CodeRabbit inference engine (AGENTS.md)
Follow WordPress Coding Standards (CSS, HTML, JavaScript, PHP) and inline-documentation standards at all times
Files:
website/src/lib/resources.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,ts,jsx,tsx}: Follow ESLint/Prettier standards for JavaScript and TypeScript files
Avoid unnecessary JavaScript, defer/lazy-load where possible, and prefer native blocks
Files:
website/src/lib/resources.ts
**/*.{js,ts}
⚙️ CodeRabbit configuration file
**/*.{js,ts}: Review JavaScript/TypeScript:
- Ensure code is linted and follows project style guides.
- Check for dead code, unused variables, and clear function naming.
- Validate accessibility and performance optimisations.
- Ensure tests are isolated and do not depend on external state.
- Check for descriptive test names and clear test structure.
Files:
website/src/lib/resources.ts
**/*.md
📄 CodeRabbit inference engine (.github/instructions/markdown.instructions.md)
**/*.md: Use one H1 (#) per file; keep heading levels sequential (never skip from H2 to H4)
Use fenced code blocks with explicit language tags (bash,yaml,markdown, etc.)
Keep links relative for in-repo files; verify they resolve before merging
Use1.for ordered lists and-for unordered lists
Keep all wording in UK English (optimise, organisation, colour, behaviour, analyse)
Do not add areferences:frontmatter field — use inline links or a footer section instead
Blank lines before and after headings, code blocks, and block-level elements
Maximum line length: 120 characters (soft limit; prefer wrapping at natural sentence boundaries)
All.mdfiles in this repository should include YAML frontmatter with required fields: file_type, title, description, version, last_updated, owners, tags, status, stability, domain
Every image (![]()) must have descriptive alt text explaining the image's purpose, not its appearance. Empty alt (![ ]()) is valid only for purely decorative images
Link text must describe the destination — never use 'click here', 'read more', or bare URLs as visible text
Every table must have a header row (| Header |). Avoid merged cells
Use headings to communicate document structure, not for visual styling
Do not rely on colour alone to convey information in diagrams or callout blocks
Mermaid diagrams must includeaccTitleandaccDescrattributes for accessibility
Specify language in frontmatter; use plain language, avoid jargon where possible
Files:
CHANGELOG.md
CHANGELOG.md
⚙️ CodeRabbit configuration file
CHANGELOG.md: Review CHANGELOG.md:
- Confirm entries follow Keep a Changelog 1.1.0 format.
- Each entry under [Unreleased] must include a PR link and issue link.
- Verify entries use the correct section headings (Added, Changed, Fixed, Deprecated, Removed, Security, Documentation, Performance).
- Check UK English spelling throughout.
Files:
CHANGELOG.md
**/package.json
⚙️ CodeRabbit configuration file
**/package.json: Review package.json:
- Check for security vulnerabilities and outdated packages.
- Ensure scripts are documented with clear, descriptive names.
- Validate semantic versioning and proper version pinning.
- Confirm devDependencies vs dependencies separation.
- Ensure scripts follow org standards (lint, test, build, format).
Files:
website/package.json
🔇 Additional comments (13)
website/package.json (1)
24-24: LGTM!website/src/components/AwesomeGithub/Icon.astro (1)
1-40: LGTM!website/src/lib/resources.ts (1)
50-57: LGTM!website/src/components/AwesomeGithub/AwesomeGithubNav.astro (1)
64-66: LGTM!Also applies to: 258-261, 329-329
website/src/pages/getting-started.astro (2)
345-347: LGTM!
112-127: Phosphor icon assets: re-check the lookup path (looks likenode_modulesisn’t present here).The script run against
node_modules/@phosphor-icons/core/assets/regular/*.svgreported all four icons (robot,books,cooking-pot,book-open) as missing—but that’s a property of the environment’s filesystem layout rather than (necessarily) icon-name correctness. In this repo snapshot, thenode_modules-based assets path may not exist, so “NOT FOUND” doesn’t reliably indicate misspellings.Please adjust verification to locate the actual installed Phosphor asset directory (or confirm
Iconmaps these names to existing glyphs during the build).website/src/pages/cookbook/[slug].astro (1)
51-51: LGTM!Also applies to: 178-178
website/src/pages/learning-hub/index.astro (2)
283-285: LGTM!
12-12: Re-check Phosphor regular icon assets for Learning Hub slugs (gear,shield-star,check-circle,robot).
The provided file-based script reports all four SVGs as missing (✗ ... NOT FOUND), but that check depends onnode_modules/...being present in the environment; re-run after installing dependencies to ensurewebsite/src/pages/learning-hub/index.astro(lines 12/35/65/95) actually resolves to real@phosphor-icons/core/assets/regular/{name}.svgfiles.website/src/pages/learn/index.astro (2)
169-171: LGTM!
10-13: Looks good: all Learn track icon names exist in Phosphor “regular” (incl. fallback)
website/src/components/AwesomeGithub/Icon.astrodefaults toweight="regular", so the Learn page’scompass,shield-star,check-circle,robot, and fallbackbook-openall correctly map to existing files under@phosphor-icons/core/assets/regular/*.svg— no icon-name surprises.Also applies to: 44-44
website/src/pages/learn/[track]/index.astro (1)
150-150: Note: Icon colour differs from other pages.This page uses
color: var(--c-light-blue)for the track icon, whilst most other migrated icon containers usecolor: var(--accent). This may be intentional for design consistency with the hero gradient, but worth confirming.website/src/pages/c/[type]/index.astro (1)
53-53: ✅ Resource type icons are already valid Phosphor icon names (no emoji hiding in resources.ts).
website/src/lib/resources.tsdefinesiconas:robot,book-open,chat-text,sparkle,shield-check,gear,puzzle-piece,wrench.- No non-ASCII/emoji-like characters were found in those icon values, and the names match Phosphor’s icon catalogue—so
currentType.iconshould render correctly.
…con CSS Extract the duplicated trackIcons mapping from learn/index.astro and learn/[track]/index.astro into a shared TRACK_ICONS constant in lib/learn.ts so both pages stay in sync from a single source of truth. Also update .ag-type-icon CSS from font-size: 32px to display: flex + color: var(--accent), consistent with all other migrated icon containers in this PR. Addresses CodeRabbit review comments on #843. https://claude.ai/code/session_01VaY86RbnELdWvFyBdEMwLH
Linked issues
Relates to #841 (Astro rebuild — follow-up icon and mobile nav fixes)
Changelog
Added
Icon.astrocomponent: reads Phosphor SVG files at build time viafs.readFileSync, embeds inline viaset:html— zero runtime JS, pure SSG output.@phosphor-icons/coredependency for SVG asset files.Fixed
AwesomeGithubNav.astro): addedz-index: 99so the fixed-position menu renders above page content instead of behind it.document.body.style.overflow = "hidden"scroll-lock on open, cleared on close — prevents background scroll while menu is open (matches homepage nav behaviour).Risk Assessment
Risk Level: Low
Potential Impact: Incorrect SVG path would render a blank icon rather than crashing; icon CSS changes are isolated to
.ph-iconwrapper and existing container selectors. Mobile nav z-index change only affects viewports ≤768px.Mitigation Steps:
npm run buildinwebsite/completes with 244 pages, zero errors.font-sizetodisplay: flex; color: var(--accent)to correctly size and colour SVGs.How to Test
Prerequisites
npm ciinwebsite/Test Steps
npm run build— confirm 244 pages, no errors./c/agents/— resource type icons render as crisp SVGs (robot, book-open, etc.), not emoji./learn/— compass, shield-star, check-circle, robot icons on track cards./getting-started/— "What next?" cards show robot, books, cooking-pot, book-open icons.Edge Cases to Verify
currentColorcorrectlyChecklist (Global DoD / PR)
aria-hiddenon icon wrappers)currentColor, same contrast as beforeset:htmlfrom package assets (not user input)https://claude.ai/code/session_01VaY86RbnELdWvFyBdEMwLH