Repository navigation
Use React Router framework mode + SPA mode - #3400
Open
david-crespo wants to merge 5 commits into
Open
david-crespo wants to merge 5 commits into
david-crespo wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
david-crespo
commented
Oct 7, 2026
| q(api.instanceView, { path: { instance }, query: { project } }) | ||
| ) | ||
| return null | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
This was just unused, apparently — this was not a route file at all.
david-crespo
commented
Oct 7, 2026
| export const clientLoader = projectLayoutLoader | ||
| export async function clientLoader({ params }: Route.ClientLoaderArgs) { | ||
| return projectLayoutLoader(params) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
This is so we can check the params against Route.ClientLoaderArgs
david-crespo
commented
Oct 7, 2026
| export const handle = makeCrumb( | ||
| (p) => p.antiAffinityGroup!, | ||
| (p) => pb.antiAffinityGroup(getAntiAffinityGroupSelector(p)) | ||
| ) |
Collaborator
Author
There was a problem hiding this comment.
I have a followup PR that typechecks these handles against the route params, avoiding the !, but it's gnarly and hacky TypeScript so I wanted to keep it separate.
Contributor
|
Visual regression test against main if not done already would be a good extra validation. But I am very excited to get hot reload again! |
david-crespo
added a commit
that referenced
this pull request
Oct 9, 2026
While doing #3400, I realized I wanted a way to test the CSP from Nexus against our actual production bundle. I decide that because CSP is enforced in the browser, we don't need Nexus for this at all, we just need the CSP string. So all we have to do is use regex to extract that from the pinned Omicron commit, and then we can test the CSP with `vite preview` and a Playwright test. I was able to confirm the test fails when it's supposed to: on the #3400 branch, if I comment out the step that moves React Router's inline bootstrap scripts into separate files, the app never renders and the test fails with the browser's CSP errors: ``` Error: expect(received).toEqual(expected) // deep equality - Array [] + Array [ + "Executing inline script violates the following Content Security Policy directive 'default-src 'self''. [...] The action has been blocked.", + [3 more of those] + "CSP violation: script-src-elem inline", + "CSP violation: script-src-elem inline", ``` Once I had that in place, I thought about what else we might want to test about the contract with Nexus, and I remembered that I have sometimes forgotten to add a new route handler on the Nexus side when we added a new top-level route prefix in the console. So I added a test that similarly extracts the console routes from Nexus and makes sure all the client-side routes are covered by those handlers. That immediately turned up a bug (go to https://oxide.sys.r3.oxide-preview.com/images, click an image, and then refresh — 404), which is fixed in oxidecomputer/omicron#11467. So the test will fail until that is merged and omicron is bumped. This is nice and neat because #3402 pulls out the setup.
Collaborator
Author
|
Visual regression test is clean, just some weirdness with timing around UUID truncation moving things by a pixel. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://reactrouter.com/how-to/spa
The behavior and basic architecture of the console is unchanged: we're still a fully static SPA served by Nexus. This PR just changes how our route files are hooked up to the router. The point of this is to
Hot reloading was broken because our route modules export
clientLoaderandhandlenext to the component. React Fast Refresh only accepts modules whose exports are all components, so editing any page triggered a full reload. The framework's Vite plugin knows about route exports and hot-updates them itself (RR docs on supported exports). We could have fixed hot reloading without using framework mode, but it would have be just as long of a PR and we wouldn't get the typegen.Changes
app/routes.tsx(JSX<Route>tree withlazy+convert) toapp/routes.tsusing the framework route helpers. Routes that had an inlinehandle, an index redirect, or a reused component with a different crumb now point at small modules inapp/routes/, since in framework mode a route'shandleand loader have to come from the module.clientLoaderuses the generatedRoute.ClientLoaderArgsand destructures the keys it needs instead of calling agetXSelectorhelper. We destructure these params on purpose in order to pass on only what we need:paramsalso holds child-route params, and two query builders (routeList,instanceNetworkInterfaceList) pass their argument through as the API query, so passingparamswhole would add e.g.route=xto the request and the cache key and break the prefetch. AGENTS.md has a line about this.pbhelpers use typedhref(), so paths and params are checked against the route config. We could in theory ditchpbaltogether and just inline the paths, but I think it's still worth keeping.index.htmlis gone.root.tsxrenders the document, andnpm run buildprerenders it at build time. This requires binding a localhost port during the build (noted in AGENTS.md for sandboxed agents).buildEndstep (tools/externalize-spa-scripts.ts) moves each one into a hashed file underassets/, andentry.client.tsxempties those tags back out after they run so hydration sees the DOM shape React Router expects.dist/tobuild/client/. React Router always writes the client build to<buildDirectory>/client, so CI, Vercel,npm run preview, and the docs now point there. The tarball is built from inside that directory, so the layout Nexus sees is unchanged. Sourcemaps still ship, same as main.routeDiscovery: 'initial': by default, framework mode uses lazy route discovery. The initial page load gets only the route manifest entries for the current URL, and the client fetches the rest from a/__manifestserver endpoint as you navigate. That's an optimization for apps with a lot of routes. We don't have that many, and Nexus doesn't serve that endpoint anyway, soinitialmode puts the whole manifest in the initial load (see the bundle comparison below for its size).app/(plus the design system), because watched Markdown elsewhere in the repo was triggering full reloads. This drops only.shadow,.!filter, and.[commit:string], which were picked up from words inflake.nix, an e2e test, and a deno tool. This could be done separately from this PR but it was relevant here because it reduces the number events that trigger reloads.loaderexport inOxqlMetric.tsx. It was dead on main too: added in Instance Metrics using OxQL #2654, but nothing ever imported it.API_MODEerror message, which said the default ismswwhen it'snexus.How we checked it
Opus 5.5 did the following during review:
app/routes.tsagainst main'sroutes.tsxroute by route: nesting, index redirects, crumbs, IDs on reused modules, and the oldelement={null}routes. Everything matches.path-builder.spec.tsnow builds its routes from the real framework config and module handles instead of the old JSX tree.ProjectsPage.tsx, which exportsclientLoaderandhandle, with the create form open. It hot-updated without a full reload, and text typed into the form survived.theme.e2e.tscheck thattheme-init.jssets the theme before React loads. They do this by blocking the app's entry script, which is nowentry.client.tsxinstead ofmain.tsx. On main, the tests confirmed the block worked by asserting that#rootwas empty, but the framework's document has no#root. Now they wait for the blocked request to fail instead. If the entry file is renamed again, the block stops matching and the tests fail, rather than letting React load and passing without testing anything.Bundle compared with main (Nexus build)
*-client-loader-*chunks and 24*-main-*chunks, so a route's loader can start fetching before its component code arrives. About 34 more are the tiny route stubs inapp/routes/. Most are under 1 KB.splitRouteModules: falseturns splitting off if the file count is a problem.routeDiscovery: 'initial'.Known dev-only quirk
Index routes that re-export a tab's
clientLoader(vpc-index,sled-index,inventory-index,alerting-index,affinity-create) keep the old loader after you edit that tab until a full reload.Typechecking breadcrumb params against each route's params will come in a followup PR.