[donbr]

Builds on #9 (merged). Targets main. Addresses the non-blocking review items left on #7:

1. bc60483 Menu closes on any navigation. It previously closed only on link click, so back/forward left it open. Open state is now tied to the location.key it was opened at, so any router navigation closes it, with no extra effect. Per-link close handlers remain for the homepage anchor handler.
2. bc60483 Focus returns to the menu button after Escape, instead of staying on a link that just unmounted.
3. b388c4b Homepage hash updates go through React Router. The smooth-scroll handler used window.history.pushState, which the router never sees. That left location.hash stale (the nav underline lagged), and back/forward across those entries wasn't treated as navigation, so item 1 couldn't fire. It now calls navigate({ hash }); the smooth scroll is unchanged.
4. 10bc243 No duplicate tab stop on project cards. "Deep Dive" was a second link to the same URL as the stretched title. It stays as visible text (aria-hidden, under the card overlay), so clicking it still opens the detail page.
5. fcaa700 postbuild script. The inline node -e one-liner becomes scripts/copy-404.mjs, with a comment explaining why GitHub Pages needs the SPA fallback.

Test plan

Production build served by vite preview, driven with Playwright:

• npm run build passes and logs the 404.html copy; dist/404.html matches dist/index.html
• npx tsc -p tsconfig.app.json --noEmit passes; npm run lint 0 errors (same 3 pre-existing warnings)
• 390px: open menu, focus a link, press Escape → menu closed, aria-expanded="false", focus on the menu button
• 390px: tap About → URL /#about, menu closed, About scrolled to top; reopen menu, press browser Back → URL /, menu closed (it stayed open before b388c4b)
• 1280px: click Publications → URL #publications, nav underline on Publications, section scrolled to top
• Projects page: a card with a detail page and a demo has two tab stops (title, Live Demo); clicking the Deep Dive text opens /assets/projects/cytoscape; Live Demo opens the demo; no nested anchors

Not included

Other review notes on #7, left for later: sitemap.xml lastmod dates are hardcoded; renderNavLink takes four positional strings; the card overlay's rounded corners ignore the 2px top border (cosmetic). Links from other pages to /#about still do a full page load.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ppa5DyJCZ1CowKtz58QARN

[claude]

Review

Clean, well-scoped follow-ups that match the codebase style, and the PR description explains the reasoning well. I found one edge case worth considering plus a few minor notes.

Potential bug: menu can reopen on back/forward (Layout.tsx)

menuOpenAt stores the location.key the menu was opened at and is never cleared by navigation. It is only reset by closeMenu, which the link handlers call. Each history entry keeps its key, so returning to that entry makes menuOpenAt === location.key true again:

1. Open the menu on entry K.
2. Navigate away without clicking a nav link, for example back then forward, or a link in the page body.
3. Return to K. The menu is open again.

Clicking a nav link avoids this because closeMenu runs, which is why the test plan passes. A fix is to reset during render:

if (menuOpenAt !== null && menuOpenAt !== location.key) setMenuOpenAt(null);

This is the documented pattern for adjusting state during render, and it drops the stale key as soon as the location changes.

Minor notes

• HomePage.tsx: navigate({ hash }) pushes a new history entry even when the hash is unchanged, so clicking the same anchor twice stacks duplicate entries. Passing { replace: target.hash === location.hash } would avoid that.
• ProjectsPage.tsx: the aria-hidden "Deep Dive" span fixes the duplicate tab stop. Clicking it relies on the title link's stretched overlay covering it, so keep that in mind if the overlay CSS changes.
• scripts/copy-404.mjs: the path is cwd-relative, which is fine under npm run. Resolving against import.meta.url would make it robust to other cwds. The comment is helpful.
• Tests: the repo has no test framework, so verification is manual (Playwright). If one is added, a router-level test of the menu-close behaviour, including the reopen case above, would be a good first test.

No security or performance concerns. The reopen case is arguably non-blocking.

🤖 Generated with Claude Code

[claude]

Review

Clean, well-scoped follow-ups. The menuOpenAt === location.key approach avoids an extra effect, and the description is clear. No blocking issues.

Suggestions (non-blocking)

1. Back/forward doesn't scroll. navigate({ hash }) now creates real history entries, but scrolling only happens in the click handler. Pressing Back from /about-anchor to /expertise-anchor updates location.hash and the nav underline but won't scroll to the section. A small useEffect on location.hash in HomePage calling scrollIntoView would cover click and history navigation; the click handler could then just call navigate.
2. Repeated clicks on the same anchor push duplicate history entries. Consider navigate({ hash }, { replace: location.hash === target.hash }).
3. Click handler target (pre-existing): e.target as HTMLAnchorElement fails when the click lands on a child element of an anchor. (e.target as Element).closest('a[href^="#"]') is more robust.
4. Escape focus: menuButtonRef.current?.focus() runs whenever the menu is open, even if focus was elsewhere. Fine in practice.
5. aria-hidden Deep Dive text: fine for avoiding the duplicate tab stop; confirm the stretched title link's accessible name is meaningful on its own.
6. copy-404.mjs depends on CWD being the repo root (true under npm run). Resolving paths via import.meta.url and a clear error if dist/index.html is missing would make it sturdier.

Tests / security / performance

• No test framework is configured (per CLAUDE.md); the Playwright manual plan is thorough. Menu-close-on-navigation would be a good first unit test if one is added.
• No security or performance concerns.

🤖 Generated with Claude Code

[claude]

Review

Clean, focused follow-ups. The postbuild script extraction, the aria-hidden Deep Dive cue and routing hash changes through navigate all look right.

Potential bug: stale menuOpenAt can reopen the menu

menuOpenAt is only cleared by closeMenu/Escape/the matchMedia handler. When the menu closes because location.key changed (e.g. browser Back), the stored key stays. Navigating back to that entry then makes menuOpen true again.

Repro: open the menu at entry A, press Back to entry Z, then press Forward to A. The menu is open again. Setting state during render when the key changes, or a small effect that clears it, would fix this:

useEffect(() => { setMenuOpenAt(null); }, [location.key]);

The PR says no extra effect is needed, so the tradeoff is a trivial effect versus this edge case. Alternatively, compare against a per-open counter instead of the key.

Minor

• HomePage click handler: navigate({ hash }) on every anchor click pushes a history entry even when the hash is unchanged (clicking the same nav item twice). Consider navigate({ hash }, { replace: location.hash === target.hash }).
• scripts/copy-404.mjs resolves dist/... relative to cwd. That's fine for npm run, but import.meta.url-based paths would be sturdier.
• Cards: the "Deep Dive" cue is hidden from assistive tech, which is fine since the title link covers it. Make sure the title link's accessible name is clear on its own.
• No tests are configured (per CLAUDE.md). The Playwright checks in the test plan are good manual coverage, but the Back/Forward reopen case above isn't among them.

No security or performance concerns.

[claude]

Review

Clean, well-scoped follow-up. Deriving menuOpen from location.key closes the menu on any navigation without an extra effect, and routing the hash update through navigate fixes the stale underline. I reviewed by reading the diff and surrounding code; I did not run the app.

Worth a look

1. HomePage click handler (pre-existing, but it matters more now).
   • e.target as HTMLAnchorElement is the clicked element, not the nearest anchor. A click on a child element inside a hash link has no .hash, so the handler is skipped. (e.target as Element).closest('a[href*="#"]') would be more robust.
   • e.preventDefault() runs before the querySelector check. If the element doesn't exist, the click does nothing. Moving it inside if (element) falls back to native behaviour.
   • Clicking the same section link twice pushes duplicate history entries, so Back stays on the same hash. Consider navigate({ hash }, { replace: location.hash === target.hash }).
   • querySelector throws on a malformed hash such as #1abc. Unlikely with the current links.
2. location.key is "default" on first load. This works, since the key is only compared for equality. A short comment saying so would help the next reader.
3. aria-hidden "Deep Dive" span. Good fix for the duplicate tab stop. Screen-reader users no longer hear "Deep Dive", and the link's name is just the project title. That's acceptable, but a visually-hidden suffix on the title link would add context if you want it.
4. copy-404.mjs. Paths are relative to cwd, which is fine under npm run. Resolving them from import.meta.url would make the script safe to run from anywhere.

Conventions: Follows CLAUDE.md (Tailwind, cn, @/ alias). CLAUDE.md could mention the location-key menu behaviour and the new scripts/ directory.

Tests: There is no test framework, per CLAUDE.md. The Playwright plan in the description covers the main paths. I'd add two edge cases: a click on a nested element inside a hash link, and clicking the same hash link twice and then pressing Back.

Security / performance: No concerns. There are no new dependencies.

Nothing blocking from my side.

🤖 Generated with Claude Code

[claude]

Review

Overall a clean set of follow-ups. Deriving open state from location.key is a neat way to avoid a close-on-navigate effect. I have one edge case and a few smaller notes.

Edge case: stale menuOpenAt can reopen the menu

menuOpenAt is never cleared when a navigation closes the menu. It only reads as closed because location.key differs. If the user returns to the original entry, the menu reappears open:

1. Open the menu at key A. menuOpenAt = A.
2. Press browser Back or Forward so the location moves away and then returns to A. The menu is closed while away but reopens on return.

Link clicks are unaffected because closeMenu resets the state. Only history navigation with the menu open hits this. The Back scenario in the test plan reopens the menu at the new key first, so it doesn't cover it.

Fix options: an effect that resets menuOpenAt when location.key changes, or a render-time check that clears the state when menuOpenAt !== null && menuOpenAt !== location.key. The second keeps the no-extra-effect intent.

Smaller notes

• Bundled change: the PR description doesn't mention the matchMedia lg-breakpoint effect. It looks fine, but 1024px is hardcoded and has to stay in sync with Tailwind's lg.
• Escape handler: it calls setMenuOpenAt(null) directly, but closeMenu() is available. Use it for consistency.
• HomePage: navigate({ hash }) pushes a history entry even when clicking the already-active hash, so repeated clicks stack duplicate entries. Consider { replace: location.hash === target.hash }. Also confirm the handler calls preventDefault() earlier in the function (not visible in the diff). Otherwise the native hash jump and the router push would both fire.
• Deep Dive span: aria-hidden is reasonable since the title link covers the card. It's no longer focusable, which is intended.
• copy-404.mjs: it uses a relative dist/ path, which is fine under npm run.
• Tests: there is no test framework (per CLAUDE.md), so the manual Playwright checklist is the coverage. A history round-trip scenario would be worth adding to it.

No security or performance concerns. Nice work.

🤖 Generated with Claude Code

[claude]

Review

Overall this is a clean, well-scoped follow-up. The render-time reset of menu state (keyed on pathname+search+hash+key) is the idiomatic way to avoid an effect, and routing the hash updates through navigate fixes the stale-underline problem at its source. No blocking issues.

Suggestions

1. decodeURIComponent can throw (HomePage.tsx). A malformed hash (e.g. #%E0%A4%A) throws a URIError inside a document-level click handler. Nothing in the site produces such a link today, but a small try/catch (falling back to the raw slice) would make it robust.
2. Handler is document-wide. It also runs for every <a> on the page, including links inside content that are same-pathname hashes. That is probably intended. Anchors with target="_blank" or download aren't excluded, though, so a same-page <a href="#x" target="_blank"> would be hijacked. Consider if (anchor.target && anchor.target !== '_self') return;.
3. Deep Dive affordance (ProjectsPage.tsx). Converting it to an aria-hidden span is a reasonable fix for the duplicate tab stop. It does drop the link's hover colour change, so mouse users lose the hover cue on that text. Putting group-hover:text-blue-800 on the card would restore it, if the card has a group class.
4. Escape focus handling. Focus only moves to the button when focus was inside the menu. That's correct. Because the menu is hidden rather than unmounted, hidden removes focus anyway, so the behaviour is fine as written. The comment could say so.
5. Tests. There's no test framework, so the Playwright checks in the PR description are the only coverage. That's acceptable here, but the menu-close-on-back/forward behaviour is the kind of regression a small e2e test would guard.

Nits

• scripts/copy-404.mjs is good. Consider failing with a clear message if dist/index.html is missing (the default ENOENT is already fairly clear, so this is optional).
• setMenuOpen/closeMenu are recreated each render, which is fine at this size.

LGTM with the minor items above.

[claude]

Review of PR 11

Clean set of follow-ups. The render-time state reset in Layout is idiomatic and avoids an effect, routing the hash update through navigate fixes the stale nav state, and copy-404.mjs is a good change. I didn't run the app; this is based on reading the diff.

Suggestions

1. decodeURIComponent can throw (HomePage.tsx). A malformed hash such as %E0%A4%A raises URIError in the click handler. The link still works because preventDefault hasn't run yet, but it logs an uncaught error. Wrap it in try/catch and bail out on failure.
2. Lost hover affordance on Deep Dive (ProjectsPage.tsx). The old link had hover:text-blue-800 and the span has no hover state. Please confirm the card or title gives hover/focus feedback (for example group-hover:text-blue-800).
3. Style nit: the Escape handler uses setMenu((m) => ...) while closeMenu uses setMenuOpen, which closes over navKey. One style would be easier to follow, though behaviour is fine.
4. Minor: navigate is in the effect deps, so the click listener may be re-attached on location changes. Harmless.
5. Tests: no framework is configured, matching CLAUDE.md, and the manual Playwright plan is thorough. If Vitest and Testing Library are added later, back/forward closing the menu and Escape focus return are good first tests.
6. Docs: CLAUDE.md doesn't mention the postbuild / scripts directory. A one-line note under Building would help.

No security concerns. The closest('a') change, origin/pathname checks and modifier-key handling look correct.

🤖 Generated with Claude Code