[donbr]

Summary

Addresses the four P0 items from the design review (one commit each):

1. Deep links and refreshes 404 on GitHub Pages (262ee64): BrowserRouter routes such as /assets/projects and /concepts returned GitHub's "Page not found" when opened directly. A postbuild script now copies dist/index.html to dist/404.html; public/sitemap.xml lists the Projects, Concepts and detail routes.
2. Navigation overflowed at phone width (6bce9d9): below lg (1024px), a menu button (aria-expanded, aria-controls, closes on Escape and on navigation) reveals a stacked link list. Nav items are defined once and shared by both layouts. Desktop styling is unchanged apart from tighter spacing/text between lg and xl so links no longer wrap at 1024px.
3. Missing social preview image (d870024): index.html pointed og:image / twitter:image at /og-image.jpg, which did not exist. Adds a 1200×630 card.
4. Nested links in project cards (74ea2b6): cards with a detailUrl wrapped <a> inside <a>. The title is now the only card-level link, stretched over the card via an ::after overlay; Demo / Repository / Hugging Face links sit above it (relative z-10). The stopPropagation workaround is removed.

Test plan

Ran locally against the production build, served by a small server that mimics GitHub Pages (serves 404.html for unknown paths):

• npm run build passes; dist/404.html is identical to dist/index.html
• npx tsc -p tsconfig.app.json --noEmit passes
• npm run lint: 0 errors, same 3 pre-existing warnings (none in changed files)
• /assets/projects and /concepts render the app when loaded directly
• 390px: no horizontal scroll; menu opens, Escape closes it, tapping a link navigates and closes it
• 1024px and 1280px: all nine desktop links on one line, menu button hidden
• Card overlay: clicking a card body opens its detail page; clicking Live Demo hits the demo link
• Document contains no nested anchors (a a count is 0)
• After npm run deploy: open a deep link on donbr.github.io, and re-scrape the URL in LinkedIn Post Inspector to confirm the preview image

Known limitation

GitHub Pages serves 404.html with HTTP status 404. Browsers render the page normally, but search engines may not index the deep routes. Fixing that would mean prerendering a static HTML page per route; out of scope here.

Deployment

The repo has no deploy workflow; merging doesn't publish. Run npm run deploy from main after merge.

Update: Concepts page removal

5ad9825 removes the Concepts nav item and the /concepts sitemap URL. The page itself (generated in b03de6c, opening with a fabricated anecdote) and its route are deleted in #8. Merge this PR before #8; in the other order the nav briefly links to the 404 page. Both orders merge cleanly; the combined build passes with no remaining references.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ppa5DyJCZ1CowKtz58QARN

[claude]

Review

Overall: a clean, well-scoped PR. Consolidating the nav into one navItems array is a good simplification, and the stretched-link card fixes the nested-anchor problem properly. I read the diff only and didn't run the build. Notes below, roughly by priority.

Suggestions

1. Mobile menu doesn't close on non-click navigation (Layout.tsx). It only closes through onClick on the links. Browser back/forward, or any programmatic navigation, leaves it open. Closing on route change is more robust:

   useEffect(() => setMenuOpen(false), [location.pathname, location.hash]);

   The same effect would let you drop the per-link closeMenu handlers.

2. Focus handling on Escape. After Escape closes the menu, focus stays on whichever link was focused, and that link has just unmounted. Returning focus to the toggle button (via a ref) would be a nice a11y touch.

3. Hash links do full page loads (pre-existing, but now more visible). <a href="/#about"> reloads the whole app on every nav click. Using <Link to={{ pathname: '/', hash: '#about' }}> gives client-side navigation. It also fixes active-state tracking, since location.hash updates without a reload. Note that React Router won't scroll to the anchor by itself, so you'd need a small effect for that.

4. Redundant links in project cards. With a detailUrl, a card now has the title link, the stretched overlay and a separate "Deep Dive →" link, all pointing to the same place. Screen-reader and keyboard users hit two tab stops for one destination. Consider dropping the "Deep Dive" link, or giving it tabIndex={-1}/aria-hidden. The after:rounded-lg overlay also doesn't account for the border-t-2, which is cosmetic only.

5. Sitemap.

   • All lastmod values are hardcoded to the same date and will go stale. Consider generating them at build time, or omitting lastmod.
   • Check that every route in App.tsx is covered. I didn't see /assets/projects/gdelt in the added entries.
   • As your known-limitation note says, these URLs are served with HTTP 404, so crawlers may ignore them. The sitemap may not help much until prerendering lands.

6. postbuild script. node -e "require('fs')..." works but is opaque. A tiny scripts/copy-404.mjs would read better. Note postbuild runs after npm run build, so a direct vite build won't produce 404.html. That's fine given that predeploy calls npm run build.

7. OG image. The og:image URL should be absolute (https://donbr.github.io/og-image.jpg) for scrapers. Please confirm index.html uses that form. Also check the JPG's file size, since LinkedIn prefers it small.

Nits

• Whitespace-only changes in the footer anchors, plus the trailing newline, add some diff noise. They're harmless.
• renderNavLink takes four positional string args. An object parameter, or a small variant: 'desktop' | 'mobile' prop, would be easier to read.

Tests

The repo has no test framework, so manual verification is what's available, and your checklist is thorough. If one is added later, the nested-anchor check (a a count is 0) and the menu's open/Escape/close behavior would be good first component tests.

Security / performance

No concerns. rel="noopener noreferrer" is present on external links, and the changes are static and add no meaningful runtime cost.

Nice work. None of the above is blocking, though #1 is worth fixing before merge.

🤖 Generated with Claude Code

[claude]

Review

Clean, well-scoped PR. The shared navItems array removes a lot of duplication, and the stretched-link card fixes the nested-anchor bug properly. Notes below, none blocking.

Correctness / possible issues

• Sitemap vs. 404 status: GitHub Pages serves 404.html with HTTP 404, so every deep URL in the new sitemap answers 404 to crawlers, and Google generally won't index those. You list this under Known limitation. Copying index.html into per-route directories (e.g. dist/assets/projects/index.html) would return 200 and make the sitemap meaningful.
• Sitemap coverage: /assets/projects/gdelt is a routed page (see CLAUDE.md) but isn't listed. Intentional? lastmod is hardcoded to the PR date for every URL and will go stale.
• Hash links cause a full reload: href="/#about" was already like this, but the shared renderNavLink is a good place to fix it. When already on /, these links reload the page. A react-router Link with a hash plus scroll handling would avoid that.
• Mobile menu state: it closes on link click and Escape, but not on browser back/forward. A useEffect keyed on location.pathname/location.hash would cover that. Consider returning focus to the toggle button after Escape.
• Duplicate links in cards: with a detailUrl, a card now has both the title link and the Deep Dive link to the same target, which is redundant for screen-reader users. Consider aria-hidden and tabIndex={-1} on one of them. cursor-pointer was also dropped from the card, so please check the cursor visually.
• postbuild: node -e with require works but is terse. A small scripts/ file would be easier to read and extend.

Performance / security

• No concerns. The keydown listener is only attached while the menu is open and is cleaned up. External links keep rel=noopener noreferrer.
• Please check the size of og-image.jpg (ideally under 300KB) and that index.html uses an absolute URL for og:image, since scrapers require that.

Tests
There's no test framework (per CLAUDE.md), and the manual test plan is thorough. If one is added later, the nav toggle/Escape behaviour and a no-nested-anchors check are good first candidates.

Nits

• The whitespace-only footer changes add small diff noise.
• Thanks for documenting the merge order with perf: code-split routes and add per-route titles #8.

🤖 Generated with Claude Code

[donbr]

Superseded by #9, which combines #7 and #8 into one PR (same changes, plus the Concepts page removal as a single commit).