Migrate to Astro 6 (phase 1)#308
Conversation
✅ Deploy Preview for eth-forkcast ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Replace the Vite + React Router SPA with an Astro 6 static foundation that
owns routing, layouts, document head, metadata, redirects, sitemap, and the
build/preview/check commands. React page bodies hydrate as client:only islands.
- astro.config.mjs: output static, site, @astrojs/react + @astrojs/sitemap,
Tailwind v4 Vite plugin; configured redirects for legacy aliases and
pending PR-only EIPs (/eips/{id} -> GitHub PR)
- Astro pages for all canonical routes; getStaticPaths for EIPs (canonical
only), calls (detail + concrete type scopes), and devnets (local specs +
active networks)
- Build-time snapshots (upcoming calls, ethPandaOps networks) shared by
getStaticPaths and the hydrated islands so links never point to un-emitted routes
- eip-stage-changes.json emitted by an Astro endpoint via a shared pure helper
- Remove react-router-dom in favor of a small navigation shim; provider-less
cross-island theme store; per-call search via a window event
- Astro-owned head/meta/theme bootstrap/initial pageview; polished 404 page;
robots points at sitemap-index.xml
- Drop SPA scaffolding: index.html, App, main, useMetaTags, generate-static-pages,
generate-eip-stage-changes, public/_redirects, public/404.html
1ef39f0 to
24753b0
Compare
|
Blocking: the migration drops the existing Please preserve these as static redirects during the Astro migration. A clean fix would be to derive a call alias redirect map from |
Incorporate the review feedback and cherry-pick organizational patterns from the alternate implementations, guided by docs/astro-migration-phase-1.md and our coding principles. Route invariant / correctness: - calls/[type]: stop emitting one-off pseudo-series scope pages (/calls/one-off-*); concrete-series scopes and one-off *call* pages are unaffected - calls/[...path]: emit an upcoming-call watch page only when the index links it internally (shared hasUpcomingWatchPage predicate); video-less upcoming calls link out to GitHub, so no unreachable routes are generated - CallPlanPage: fix a /calls/acdt/66 404 — route the "last discussed" link through a shared formatCallReference helper that zero-pads to the emitted /calls/acdt/066 (de-duplicated out of EipNotice/EipTimeline) - StakeholderUpgradePage: guard the back-link via getUpgradePagePath Single source of truth (no build-vs-runtime drift): - extract the upcoming-call issue parser and YouTube-URL extractor into one pure module (domain/calls/upcomingCallParsing.ts), shared by the build snapshot script and the runtime fetch path instead of hand-synced copies - extract pure devnet-network derivation into domain/devnets/networks.ts; the React hook is now a thin wrapper over it Organization / cruft: - typed page-metadata registry (domain/routes/pageMetadata.ts) replaces the inline title/description repeated across the static pages - richer EIP <title> (include the layman title) via existing helpers - fold the generic lib/ bucket into domain/calls + components/ - drop the unused vite and @vitejs/plugin-react devDeps Verified: lint, 44 tests, astro check (162 files, 0 errors), build (815 pages), and browser spot-checks (canonical routes, legacy redirect, real 404 with nav, theme toggle, devnet route, and zero-padded agenda call links).
|
@dionysuz-bot this round incorporates the review feedback and cherry-picks organizational polish from the alternate implementations. Requesting a re-review. Route invariant / correctness
Single source of truth (no build-vs-runtime drift)
Organization / cruft
Verified locally: lint, 44 tests, |
|
No blocking findings. I re-reviewed the route invariants, shared upcoming-call parsing, call-reference padding, devnet snapshot derivation, and the Astro metadata/navigation changes. The current approach looks consistent with the stated intent and with the project’s preference for explicit domain helpers over hand-synced route/link logic. Verified locally after installing dependencies from the lockfile: |
The SPA matched a bare issue number and redirected it to the call's canonical
/calls/{series}/{number} page (the old CallPage issue-redirect effect); the
migration had dropped this, 404ing ~199 previously-shared URLs. Restore it as
build-time Astro redirects derived from the compiled call data via one pure
helper (src/domain/calls/callRoutes.ts), so the alias set stays in sync with the
pages the build emits and can never drift — the same pattern as the pending-EIP
redirects. One-off calls are handled uniformly (e.g. /calls/1954 ->
/calls/one-off-1954/001, the friendly entry point for a series-less call).
Add a unit test asserting every completed call with an issue keeps a unique
alias, so a future call sync cannot silently drop or clobber a shared URL.
Update the plan and intentional-feature-removals docs to match (these aliases
are now a preserved redirect, not a removed route).
|
@dionysuz-bot follow-up: this restores the
Verified: 47 tests, lint, Requesting a re-review. |
|
No blocking issues found. The follow-up addresses my earlier alias concern. I couldn’t run the targeted Vitest test locally because this checkout does not have dependencies installed ( |
Route pageview tracking moved into the Astro shell (Layout.astro fires the initial trackPageView); the hook's trackPageView has no remaining callers. Removing it avoids future re-wiring that would double-count. The shell stays the single pageview source; the hook keeps interaction-event tracking (trackEvent / trackUpgradeView / trackLinkClick).
|
@dionysuz-bot follow-up: removed the dead I also evaluated the
Verified: lint, 47 tests, |
|
Re-reviewed the follow-up and related Astro shell/config paths. No blocking issues found. The pageview tracking now has a single owner in I could not re-run lint/tests in this checkout because dependencies are not installed ( |
|
the navbar logo has an undesirable flicker. consider: |
…ct polish - Convert the site nav/logo from a `client:only` React island to a static `SiteNav.astro`, fixing the logo flicker: the header, links, and wordmark now render in the initial HTML (fixed dimensions, prefetched links). Theme toggle, upgrade dropdown, mobile menu, and call-search button are wired by a small client script; the now-unused Logo/SiteNav/ThemeToggle/ThemeContext React modules are removed. Enable Astro `prefetch` and mark main nav links. - Remove the dead non-embedded branch of StakeholderUpgradePage (the stakeholders view is only ever embedded, so the standalone shell + back-link were unreachable) and drop the now-vestigial `embedded` prop. - astro.config: fold the redirect builders' try/catch into one `readCompiledData` helper, and rename `legacyRedirects` -> `aliasRedirects`. - Trim process/marker cruft from comments across the PR (no "Phase 1", "the bot"), keeping references that explain why current code is shaped as it is; rewrite the comments that described the nav as a React island.
d574ead to
de8a048
Compare
|
Done — the navbar flicker is fixed. Converted One deviation from step 2: instead of keeping the interactive pieces as hydrated React islands, I wired them with a small vanilla
I also updated the comments that described the nav as a React island ( Verified in a browser against the built output: logo + links in the initial HTML (fixed dims), theme toggle persists + swaps icon + restores, upgrade dropdown opens on hover/focus with |
Fold the route-change catalog into docs/astro-migration-phase-1.md under a new "Intentional Route Changes" section, so there's a single doc to keep under docs/ for posterity. Repoint the in-doc cross-reference and the astro.config.mjs comment at the consolidated section, and remove docs/intentional-feature-removals.md.
Drop the `readCompiledData` dedup helper and inline the try/catch in `pendingEipRedirects()` / `callIssueRedirects()` again — the original two-block form is simpler and reads fine. Keeps the comment cleanups and the `legacyRedirects` -> `aliasRedirects` rename.
The dropdown's visibility is already fully CSS-driven (group-hover / group-focus-within). Drop the five JS listeners that mirrored that state into aria-expanded — a synced attribute that could momentarily desync (e.g. Escape while hovering) and earned nothing the CSS didn't already do. Keep aria-haspopup, and keep a single Escape handler that closes the menu by blurring focus out.
Per review, `legacyRedirects` reads more clearly: these are legacy aliases that we can still extend. Comment updated to match.
|
A few changes I’d like in this PR:
Also, add a small section in astro-migration-phase-1.md for phase 2 / subsequent PRs. under that, include:
|
…vigate> The helper is not a router; it is same-page URL state plus real browser navigation for path changes. Rename `navigation.tsx` -> `browserLocation.tsx` so the name reflects that boundary, and update all importers. Remove the `<Navigate>` declarative-redirect export and its two call sites (EipPage, DevnetSpecPage). Client-side route normalization belongs to Astro now: getStaticPaths emits only valid detail pages, configured redirects handle aliases, and unknown paths fall through to Astro's 404 — so those fallbacks were unreachable. Also drops DevnetSpecPage's now-dead `loading` guard (the snapshot hook resolves synchronously).
Keeping the previous snapshot on a fetch failure is right for local/offline dev, but CI/deploy builds shouldn't silently ship stale or empty upcoming-call / devnet route data. Fail when CI=true; set ALLOW_STALE_ROUTE_SNAPSHOTS=1 to opt back into the lenient behavior.
Captures the deferred cleanups: replace temporary <Link> with <a> for plain cross-page links, drop the React Router compat props, keep narrowing the browser-location helper, and move query/hash ownership into Astro primitives as routes migrate.
|
Agreed with all four — done and pushed (4aa891d, ff94c2a, 733bcf4). Rename the URL helper. Remove Stricter snapshot in CI. Phase 2 doc section. Added a "Phase 2 and Beyond" section to Verified: lint, 47 tests, |
| @@ -0,0 +1,172 @@ | |||
| /* eslint-disable react-refresh/only-export-components -- browser-location helpers (hooks + Link) intentionally share one module */ | |||
There was a problem hiding this comment.
@dionysuz-bot i don't really like this name browserLocation.tsx. we had navigation.tsx before, which looks better to me. if it's too wide, maybe we could do navigationHelpers.tsx, but i think navigation.tsx is actually best. what do you think?
…harden helpers + tests - deploy.yml (build:fresh): pass GITHUB_TOKEN so the build-time GitHub API snapshot fetch uses the authenticated rate limit; an unauthenticated 60/hr cap (CI sets FAIL_ON_STALE) could otherwise fail a deploy despite a committed snapshot. (#311 already removed the fetch from the non-deploy build, so only build:fresh needs it.) - browserLocation: compare normalized pathnames in useNavigate so a trailing-slash directory URL (/calls/) and a no-slash target (/calls) stay same-path and keep the in-place query update instead of a full reload. Note Link's ignored `replace`. - EipPage: drop the cross-EIP tab-reset effect that's dead under Astro full-reload nav (keep scroll-to-top). - callReference: throw a descriptive error on a malformed "{series}/{number}" ref instead of a cryptic padStart crash; cover with a test. - upcomingCalls.test: add behavior tests for the route-deciding parsers (parseUpcomingCallFromIssue, extractYouTubeUrl, resolveUpcomingCallNumber, hasUpcomingWatchPage). - astro.config: redirect builders rethrow non-ENOENT errors (corrupt data fails loud; missing data stays graceful). - docs: clarify call-index filter buttons (ACD scopes path-route; breakout series use dropdown query state).
|
@dionysuz-bot pushed Build reliability
Correctness / cleanups
Tests
Docs
Local gates green: |
|
Re-reviewed The follow-up addresses the prior concerns cleanly: deploy uses an authenticated GitHub API token for Local verification:
|
|
EDIT: see #312 for the fast follow migration to i'm referring to this whole part that is above the main content:
this is not blocking for implementation. at the least let's do it as a fast follow PR: so open a github issue and implement quickly (can also include the same situation on the |
|
@dionysuz-bot i don't really like this name browserLocation.tsx. we had navigation.tsx before, which looks better to me. if it's too wide, maybe we could do navigationHelpers.tsx, but i think navigation.tsx is actually best. what do you think? |
|
I agree that
I’d avoid |
@dionysuz-bot /operate land a commit on this PR that renames |
|
Done. Pushed Renamed Validation passed:
|
Clarify navigation helper's purpose and suggest renaming.


This is the phase 1 of astro migration. There will be another phase which refactors existing react components into astro idioms.
TL;DR:
There was a lot that went into the PR, you can see the initial spec at astro-phase-1-migration-plan.md. See below for a longer description of the approach.
I'd like to write quickly about how I approached this PR, please ask me for further details. I spent most of my time on the astro-phase-1-migration-plan.md. The astro migration went through several autonomous iterations, and I discovered the best approach would be to just set up the routing/skeleton with astro, and then in a follow-up PR, systematically loop through each of these routes that are at runtime hydrating the react shell on each page route, and render more useful static HTML for it with a .astro file, or at least to the degree that we can without reducing the quality of our UX (basically, be judicious about our usage of islands). The main motivation of this initiative is useful static HTML for LLMs and crawlers.
After settling on the phase 1 migration plan, I proceeded to do implementations:
ultracode(using dynamic workflows)high(using/goalloop)I cross-analyzed both implementations with ultracode and codex high/xhigh, and we all determined that the Claude dynamic workflows implementation served as a stronger foundational base (as per my injected coding principles and the intent for phase 1 migration). During this process I aggregated issues/findings into a single review artifact, and passed it to claude ultracode to resolve the findings, using my GitHub
@dionysuz-botcodex agent as backpressure on the resolution commits (the loop was basically inspect issue / reason about it -> push changes or present argument -> wait for dionysuz-bot until no blocking issues).After all issues were addressed from the ultracode loop, I did a final pass of ultracode review + three parallel codex high reviews; both reported back with ~zero blocking changes. This was really exciting to see! The ultracode reviews were multi-dimensional adversarial reviews. The codex high reviews admittedly feel poorly suited to the size of the diff, that's also why I did multiple in parallel for fuzz. A codex
/goalloop could've been better, especially if driving it in claude-dynamic-workflow fashion), but out of interest of time and for simplicity I went for multiple regular reviews.After these passes I did a human review pass, quickly reviewing every file individually in GitHub, mainly looking at structure / high-level seams and boundaries, comments, and stylistic nitpicks. I commented across the PR with my issues and had Claude ultracode resolve issues. I also worked with codex imperatively during this manual process until I clicked the checkbox on each file on GitHub as looking ~correct by my human eye and making sure I'm comfortable.
Finally I did one more review pass of ultracode claude and multi-codex to confirm no blocking issues after our changes from human review.