[donbr]

Builds on #10 (Concepts page removal, merged). Supersedes #7 and #8. One commit per change:

Broken or blocking (design review P0)

1. 33af66d Deep links and refreshes 404 on GitHub Pages. A postbuild script copies dist/index.html to dist/404.html. (This commit also added the routes to public/sitemap.xml; 78003f7 below reverts that.)
2. d7fd5d0 Navigation overflowed at phone width. Below lg (1024px) a menu button (aria-expanded, aria-controls, closes on Escape and on link tap) reveals a stacked list. Nav items are defined once and shared by both layouts; desktop links no longer wrap at 1024px.
3. 9e9112a Missing social preview image. Adds the 1200×630 public/og-image.jpg that og:image / twitter:image already referenced.
4. 415d41e Nested links in project cards. The title is the only card-level link, stretched over the card with an ::after overlay; Demo / Repository / Hugging Face links sit above it. Removes the stopPropagation workaround.

Performance and meta

5. 44f2214 Code-split routes. Home and 404 stay eager; Projects and the six project routes use React.lazy inside one Suspense boundary whose fallback keeps the Layout. Homepage script: 2,128.6 KB → 278.2 KB (678.4 → 87.0 KB gzipped).
6. e9c133f Per-route titles. Each page renders <title>Page Name | Don Branson</title>, hoisted by React 19; index.html's title stays as the no-JS fallback.
7. 05e30f6 Docs. CLAUDE.md updated for lazy routes and the title convention.

Test plan

Production build served by vite preview, driven with Playwright (earlier rounds also used a server mimicking GitHub Pages' 404.html fallback):

• npm run build passes; dist/404.html and dist/og-image.jpg present
• Every commit typechecks on its own; npm run lint 0 errors (same 3 pre-existing warnings)
• Final tree is identical to the previously reviewed combined branch (only the Concepts removal moved to Remove the Concepts page #10)
• Homepage at 390px: only the main script loads; no horizontal scroll; menu opens, Escape closes it; tapping Projects navigates, closes the menu and sets the title to "Projects | Don Branson"
• Projects page: no nested anchors; 5 cards with stretched title links; card body opens the detail page, Live Demo opens the demo
• Desktop 1024px and 1280px: all nav links on one line
• Direct loads of every detail route render with the correct title; Cytoscape, ECharts and the GDELT Leaflet map (Map tab, 9 markers) render
• After npm run deploy: open a deep link on donbr.github.io and re-scrape in LinkedIn Post Inspector

Known limitations

• GitHub Pages serves 404.html with HTTP status 404; browsers render normally but crawlers may skip deep routes. Prerendering would fix it; out of scope.
• /assets/projects/situational-awareness still loads ~1 MB of ECharts on demand.
• Review follow-ups from fix: P0 design-review items — SPA deep links, mobile nav, OG image, card links #7 (menu closes on any route change, focus return after Escape, redundant Deep Dive tab stop, postbuild script) are in the next PR in the stack.

Deployment

No deploy workflow; merging doesn't publish. Run npm run deploy from main after merge.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ppa5DyJCZ1CowKtz58QARN

[claude]

Review of PR 9

Well-scoped PR with clear commits. Notes below, none blocking.

Potential issues

1. 404.html fallback returns HTTP 404. The postbuild copy makes deep links render in browsers, but GitHub Pages serves it with a 404 status, so crawlers may not index those routes, and the sitemap lists URLs that return 404 on direct fetch. Consider prerendering if SEO matters.
2. Sitemap omits /assets/projects/gdelt although the other project routes were added. lastmod is also hardcoded for every entry.
3. Layout remounts around the Suspense fallback. RouteFallback renders its own Layout and each page renders another, at different tree positions, so the nav unmounts and remounts on each lazy navigation (resets menuOpen, possible flicker).
4. Hash links use href=/#about (full page reload). Pre-existing, but now shared by both menus; a router Link with a hash would avoid reloads.
5. Mobile menu: Escape closes it but focus is not returned to the toggle button (minor a11y).
6. postbuild uses node -e with require, which breaks if the package becomes type module. A small script file is more robust.

Good

• Lazy loading cuts the homepage bundle substantially, with Home and 404 kept eager.
• Nav items are defined once and shared; the stretched-link card pattern fixes nested anchors.
• Title hoisting via React 19 is clean, and CLAUDE.md is updated.

Tests
No test framework exists (per CLAUDE.md), so verification is manual/Playwright. A cheap CI smoke check that dist/404.html exists and all routes appear in the sitemap would help.

Nit: the CLAUDE.md line references in Project Data Management (153-242) may be stale.

🤖 Generated with Claude Code

[claude]

Review

Overall this is a clean, well-scoped PR. The commits are separated sensibly and CLAUDE.md is updated to match. I read the diff only and did not run the build. Notes, roughly by importance:

Worth considering

1. The 404.html fallback is served with HTTP 404. Deep links render correctly in a browser, but crawlers and link unfurlers see a 404 status. The sitemap now lists routes that only resolve through that fallback, so search engines may decline to index them. If SEO matters for these URLs, prerendering the few static routes into real dist/<route>/index.html files is the usual fix. If not, this is fine for a portfolio, but the sitemap entries are of limited value.
2. There is no error boundary around the lazy routes. If a chunk fails to load (offline, or a stale tab after a deploy replaced the hashed filenames), React.lazy throws and the app unmounts to a blank page. A small error boundary with a reload prompt would help.
3. The sitemap omits /assets/projects/gdelt. Every other project route is listed. Please confirm that is intentional. lastmod is also hardcoded to the PR date, so it will go stale.

Minor

• In Layout.tsx, the hash links are plain <a href="/#about"> tags. They cause a full page load from other routes. This is pre-existing, but since the nav is being refactored, <Link to={{ pathname: '/', hash }}> would be better.
• The mobile menu closes on link click and Escape, but not on other route changes (browser back). Closing it in a useEffect keyed on location.pathname would cover that. Returning focus to the toggle on Escape would be a nice accessibility touch.
• External Graph Demos ignores activeClassName and hardcodes its color. That is fine, but slightly surprising inside the shared helper.
• postbuild uses node -e "require(...)". It works, but a small scripts/ file would be more readable.
• The trailing-whitespace cleanup in Layout.tsx adds diff noise.

Good

• The stretched-link pattern removes the nested-anchor invalid HTML and the stopPropagation hack. The relative on the card and z-10 on the secondary links are correct.
• The lazy-loading win is large (2.1 MB to 278 KB). Keeping the Layout in the Suspense fallback avoids layout flash.
• The mobile nav has proper aria-expanded, aria-controls and aria-label attributes.
• Per-route <title> through React 19 hoisting needs no extra dependency.

Tests

There is no test framework (noted in CLAUDE.md), so the Playwright manual checks are the only coverage. A smoke test that visits each route and checks the title would guard the lazy-route and 404 changes.

No security concerns. External links use rel="noopener noreferrer".

🤖 Generated with Claude Code

[claude]

Review

Overall this is a clean, well-scoped PR. The commits are separated sensibly and the CLAUDE.md updates match the code. I reviewed the diff only and didn't build or run it.

Worth considering

1. 404.html fallback returns HTTP 404. GitHub Pages serves 404.html with a 404 status for deep links. Browsers render the SPA fine, but crawlers may treat /assets/projects/* as not found, which works against the sitemap you're adding. If SEO matters, pre-render per-route index.html files. If it doesn't, this is fine, and a short note in the docs would help.
2. Hash nav links do a full page load. The nav uses <a href="/#about"> (kept from the old code) instead of <Link to="/#about">. From a project page this reloads the app. It's pre-existing, but Link would be a cheap follow-up.
3. Stale chunks. The error boundary with a reload prompt is good. You could also listen for vite:preloadError and reload automatically, with a guard against reload loops.
4. No title while a lazy route loads. While RouteFallback shows, the previous page's title persists. Add a <title> to the fallback if that matters.

Minor

• renderNavLink takes four positional args, two of them class strings. An options object would read better.
• The external branch ignores the active and inactive class args and hardcodes colors, so the computed classes is unused there.
• The mobile menu doesn't close on outside click (optional).
• The postbuild one-liner (node -e with require) works, but a small scripts/postbuild.mjs would be easier to maintain.
• The stretched-link overlay prevents text selection inside cards. That's a known tradeoff.

Tests and security

• There is no test framework (per CLAUDE.md). The manual Playwright plan is thorough. If tests are ever added, the lazy loading, error boundary and resetKey reset are good smoke-test candidates.
• No security concerns. External links use rel="noopener noreferrer" and there's no new input handling.

Positives

• The bundle drop from 2.1 MB to 278 KB is a substantial win.
• The nav items are defined once and shared by both layouts.
• The ARIA attributes are done properly.
• Removing the nested links and the stopPropagation workaround is the right fix.

Nothing here blocks merging.

[claude]

Review

Solid, well-scoped PR. The commit-per-change structure, the comments explaining why, and the accessibility work (aria-expanded/aria-controls, Escape to close, stretched-link instead of nested anchors) are all good. No blocking issues found. I read the diff only and did not build or run it.

Suggestions / things to double-check

1. 404.html returns HTTP 404. The postbuild copy fixes deep links for visitors, but GitHub Pages serves it with a 404 status. Crawlers may not index those routes, which conflicts with the sitemap intent (reverted in 78003f7, so you are probably aware). Worth a note in CLAUDE.md.
2. postbuild inline node -e. It works via npm's default hooks, but a small script file or a comment would be easier to maintain.
3. Hash links do full reloads. href={/${item.hash}} in Layout.tsx is a plain <a>. Pre-existing, but with lazy routes, going from a project page back to /#about now re-downloads the bundle. <Link to={{ pathname: '/', hash }}> would keep it client-side (hash scrolling may need a small effect).
4. Mobile menu state. It closes on link click and Escape, but not on browser back/forward or other route changes. Consider useEffect(() => setMenuOpen(false), [location.pathname, location.hash]).
5. Layout remounts on every route change. Each page wraps itself in <Layout>, and RouteFallback renders another, so the nav unmounts and remounts while a chunk loads. A layout route with <Outlet/> would avoid that and remove the duplicated Layout in RouteErrorBoundary. Bigger refactor, so optional.
6. Per-page <title>. This relies on React 19 hoisting while index.html keeps a static title. Please confirm document.title updates on client-side navigation, not just on first load.
7. Error boundary. The chunk-error regex and the vite:preloadError reload guard are thoughtful. Minor: getDerivedStateFromProps comparing resetKey would avoid one render with the stale error that componentDidUpdate + setState causes.
8. Project cards. The stretched ::after overlay blocks text selection inside the card. Acceptable trade-off.
9. CLAUDE.md. The Layout section still describes only the desktop nav and should mention the mobile menu. The doc says ProjectsPage is eager, while item 5 of the PR description says Projects is lazy, so please make them consistent.
10. Tests. There is still no test framework. The new behaviors (error boundary reset, menu toggle, preload-reload guard) would suit a small Vitest + Testing Library setup. Not required for this PR.
11. og-image.jpg. Please check the file size (ideally under 300 KB) and that the og:image URL matches the deployed domain.

Nice bundle win (2.1 MB to 278 KB). LGTM modulo the small points above.

[claude]

Review

Well-structured PR: one commit per concern, clear rationale, and the bundle reduction (2.1 MB → 278 KB) is a real win. No blocking issues found.

What looks good

• Nav items are defined once and shared by the desktop and mobile layouts. The mobile menu has aria-expanded/aria-controls and closes on Escape and on link tap.
• The stretched-link card pattern removes the invalid nested <a> elements and the stopPropagation workaround.
• RouteErrorBoundary with resetKey and the vite:preloadError reload guard handle the stale-chunk-after-deploy case well.
• Docs in CLAUDE.md are updated, and the stale line-number references are gone.

Suggestions / minor issues

1. postbuild script (package.json): node -e "require('fs')..." breaks if package.json ever gets "type": "module". It's fine today, but a small scripts/ file or fs.copyFileSync via an ESM-safe form would be more robust. Also, npm run deploy relies on predeploy → build triggering postbuild, so please confirm that 404.html ends up in dist on deploy (CI will show it).
2. 404.html and HTTP status: GitHub Pages serves 404.html with a 404 status, so deep links render for users but crawlers see a 404. This is acceptable for a portfolio, but the sitemap entries (reverted in 78003f7) are rightly omitted. Worth a note in the docs.
3. Hash links in Layout: href={/${item.hash}} causes a full page load from non-home routes, and the existing behaviour is unchanged. A <Link to={{ pathname: '/', hash }}> would keep the navigation client-side. Not a regression, so optional.
4. Menu state on route change: the menu closes only via link onClick. Back/forward navigation while the menu is open leaves it open. A useEffect on location.pathname/hash would cover that.
5. CHUNK_LOAD_ERROR regex depends on browser-specific message strings. It's reasonable, but you could also check error.name === 'ChunkLoadError'-style cases or fall back to offering reload for any error. The second variant already does offer reload, so the impact is low.
6. RouteFallback wraps in <Layout> without a title, so the document title briefly reverts to the index.html default during chunk loading. This is cosmetic.
7. StatusPage import uses @/lib/utils.ts with a file extension, while the rest of the codebase uses @/lib/utils. Please make it consistent.
8. sessionStorage reload guard: the 10 s window is sensible. A one-line comment noting that event.preventDefault() suppresses the error rethrow would help future readers.
9. Trailing newline changes (export default Layout;, NotFoundPage) and whitespace-only edits in the Layout footer are noise. They're harmless.

Tests

There's still no test framework (as documented in CLAUDE.md). Manual Playwright verification is described in the PR, which is good. The new logic (error boundary reset, reload guard, mobile menu) would be good first candidates if you add Vitest + Testing Library later.

Security

Nothing concerning: external links keep rel="noopener noreferrer", and there's no dangerouslySetInnerHTML or user-controlled input. sessionStorage is wrapped in try/catch.

Overall: LGTM, with the above as optional polish.

[claude]

Review

Solid PR: the commits are well-scoped, comments explain the why, and it follows the CLAUDE.md conventions (@/ alias, cn(), direct imports for the eager pages, docs updated). No blocking issues found. I read the diff only and didn't run the build.

What looks good

• The stretched-link card removes the nested <a> inside <a> problem without stopPropagation.
• The mobile nav is accessible (aria-expanded, aria-controls, Escape to close, a target that always exists). Nav items are defined once and shared by both layouts.
• Code-splitting is a big win (2.1 MB → 278 KB). RouteErrorBoundary with resetKey and the guarded vite:preloadError reload cover the stale-chunk-after-deploy case well.

Suggestions / minor points

1. PR description is out of date. It doesn't mention RouteErrorBoundary, StatusPage, or the main.tsx reload handler. These are non-trivial and deserve a line in the description. It also says Projects is lazy, while App.tsx keeps it eager (the code comment explains this).
2. 404.html is served with HTTP 404. The SPA fallback works, but crawlers see a 404 status for deep links like /assets/projects/gdelt. That's an inherent limitation of the GitHub Pages approach. If SEO for those routes matters, consider prerendering them or using a hash router.
3. Hash links do full page loads. href={/${item.hash}} was already like this, but from the homepage it reloads the page instead of scrolling. Using <Link to={{ pathname: '/', hash }}> would be smoother, and with the Suspense setup a reload is a bit more costly. Optional.
4. Focus handling in the mobile menu. On Escape, focus isn't returned to the toggle button. If focus was inside the menu, it's lost.
5. Lazy chunk retry. After a failed lazy() import, React caches the rejected promise. Navigating away and back to the same route in the same session will hit the error again until a full reload. The "Reload page" button handles this, so it's acceptable.
6. postbuild inline require. It works with node -e, but a tiny script in scripts/ or cp would read better. This is a nit.
7. Fallback remounts Layout. RouteFallback renders its own <Layout>, so nav state (such as an open mobile menu) resets when the lazy page resolves. This is harmless but could cause a brief flicker. Hoisting Layout above the Suspense boundary would avoid it, though the pages each render their own Layout today.
8. Tests. There's no test framework, per CLAUDE.md. The Playwright checks in the test plan are good. The reload-loop guard and the chunk-error regex in the boundary would be easy unit-test targets if a framework is ever added.
9. Security. Nothing concerning. External links use rel="noopener noreferrer", and sessionStorage access is wrapped in try/catch.

Overall: LGTM, with the description update being the main ask.

[claude]

Review of PR #9

Well-scoped and well-documented PR. I reviewed the diff for CLAUDE.md, package.json, App.tsx and Layout.tsx and found no blocking concerns.

What looks good

• 404.html fallback: the postbuild copy is a standard SPA fix for GitHub Pages, and it also runs under predeploy.
• Code splitting: lazy project routes with Layout kept in the Suspense fallback (2.1 MB to 278 KB). RouteErrorBoundary with resetKey handles stale chunks after a deploy.
• Nav refactor: a single navItems list shared by desktop and mobile removes duplication. The mobile menu has aria-expanded, aria-controls and Escape-to-close.
• Per-route titles use React 19 <title> hoisting with no new dependency.
• CLAUDE.md updated alongside the code.

Suggestions (non-blocking)

1. Hash links cause full page loads. Hash items render <a href="/#about">, so clicking them inside the SPA reloads the page. This predates the PR, but the refactor is a good time to switch to <Link to={{ pathname: '/', hash }}>.
2. Close the mobile menu on location change (a useEffect on location) rather than per-link onClick. That also covers browser back/forward. The menu also stays "open" in state if the viewport is resized past lg and back.
3. isItemActive uses startsWith(item.to). It would also match a future /assets/projects-foo. Consider pathname === to || pathname.startsWith(to + '/').
4. RouteFallback wraps its own <Layout>. Layout remounts when the page replaces the fallback, which resets the menu state and may flicker the nav. Hoisting Layout above Suspense would avoid that. Minor.
5. Postbuild script: an scripts/postbuild.mjs would be easier to extend, for example to fail loudly if dist/index.html is missing.
6. Sitemap and SEO: GitHub Pages serves 404.html with an HTTP 404 status for deep links. Browsers render it fine, but crawlers may not index those routes. Please confirm the final public/sitemap.xml is what you intend, given the add-then-revert described in the PR.

Tests

There is no test framework (per CLAUDE.md), so the Playwright run in the test plan is the only coverage. If you add Vitest later, a smoke test for the menu toggle, Escape and the error boundary would be worthwhile.

Security / performance

No security concerns: external links keep rel="noopener noreferrer" and there are no new dependencies. The performance impact is clearly positive.