Skip to content

fix: don't let the menu fetch overwrite a host-supplied menu - #147

Open
isaaclombardssw wants to merge 1 commit into
mainfrom
fix/dont-refetch-menu-over-host-data
Open

isaaclombardssw wants to merge 1 commit into
mainfrom
fix/dont-refetch-menu-over-host-data

Conversation

@isaaclombardssw

Copy link
Copy Markdown
Member

Problem

useMenuItems fires an unconditional fetch on mount, so a host that passes menuBarItems gets its menu replaced by whatever /api/get-megamenu returns:

useEffect(() => { refreshData().then(setMenuItems) }, []);          // async — resolves last, wins
useEffect(() => { if (menuBarItems) setMenuItems(menuBarItems) },   // sync — immediately clobbered
          [menuBarItems]);

That endpoint sits behind three caches — a 60-min in-process cache in the route, a CDN cache, and max-age=14400 in the browser. So SSW.Website can deploy a menu change, serve it correctly in its HTML, and still show the old menu the moment hydration finishes.

Live example today: SSW.Website PR #4981 added "Claude Training Workshop" and removed "Azure Super Skills". view-source:https://www.ssw.com.au/ has the new menu; the rendered page has the old one, because the CDN is still holding a copy of the API response captured 54 minutes before that PR merged.

Fix

Skip the fetch when the host already supplied the menu — one source of truth instead of two racing ones. Hosts that pass nothing (cross-site consumers) keep the existing behaviour unchanged.

This also delivers what the same-origin branch above it was added for: a PR preview slot now actually shows the menu that slot built with, instead of production's.

Testing

Verified locally with a renderHook test — fetch not called when menuBarItems is supplied, still called and applied when it isn't. It passes with this change and fails without it.

It isn't in this PR because vitest cannot run in this repo at all on main, for four unrelated reasons, and fixing them means a ~500-line lockfile diff that would bury a 9-line fix. Raising that separately. For reference:

  1. rollup-plugin-node-externals in plugins breaks vitest's config resolution — needs apply: "build"
  2. jsdom isn't in devDependencies
  3. vite.config.ts points setupFiles at scripts/tests/setup.js; the file is setup.ts
  4. scripts/tests/setup.ts uses the pre-v6 @testing-library/jest-dom default import — needs import * as matchers

Also worth knowing: with those fixed, the pre-existing src/App.test.tsx fails. It asserts App-header and display: flex on a <header> that has no className — leftover CRA boilerplate that has never matched this component. Left alone here.

CI only runs pnpm lint, so none of this shows up as red today.

Shipping

publish.yml publishes on a GitHub release, so this needs a version bump + release cut after merge, then a follow-up PR in SSW.Website bumping ssw.megamenu from 4.14.0.

Note this does not fix the current stale menu — that needs a CDN purge of https://www.ssw.com.au/api/get-megamenu. This stops it recurring.

🤖 Generated with Claude Code

useMenuItems fired an unconditional fetch on mount, so a host that passed
menuBarItems had its menu replaced by whatever /api/get-megamenu returned.
That endpoint sits behind an in-process cache, a CDN cache and a 4h browser
cache, so SSW.Website could deploy a menu change, serve it correctly in its
HTML, and still show the old menu once hydration finished.

Skip the fetch when the host supplies the menu. Hosts that pass nothing are
unaffected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant