ssr for auth - #68
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces an SSR-capable frontend runtime (Express + Vite SSR entrypoints), adds an authentication context fed from Entra ID headers, and restructures local/prod startup/build artifacts to support a production-ready deployment flow.
Changes:
- Added SSR entrypoints (
entry-server.tsx,entry-client.tsx) and updated HTML bootstrapping for server-rendered markup + hydration. - Introduced auth typing +
AuthContextand applied role-based route filtering inApp.tsx. - Added a custom
server.js(Express + proxy + SSR) and refactored container build into a multi-stageContainerfile.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/types/auth.ts | Adds auth/role type definitions and a window.__AUTH_DATA__ global. |
| frontend/src/main.tsx | Updates legacy client bootstrap to hydrate and wrap with AuthProvider. |
| frontend/src/entry-server.tsx | Implements server-side rendering and injects initial auth data into HTML head. |
| frontend/src/entry-client.tsx | Adds client bootstrap that hydrates when SSR markup exists. |
| frontend/src/contexts/AuthContext.tsx | Adds auth context/provider with role helpers used throughout the app. |
| frontend/src/components/ClientFinchBridge.tsx | Dynamically imports Finch on the client to avoid SSR-incompatible module loads. |
| frontend/src/App.tsx | Filters routes based on roles and swaps Finch layout rendering into the client bridge. |
| frontend/server.js | Adds a unified dev/prod Express server supporting SSR and API proxying. |
| frontend/package.json | Refactors scripts for separate client/server builds and adds server/proxy dependencies. |
| frontend/package-lock.json | Updates lockfile for new dependencies and versions. |
| frontend/index.html | Adds SSR placeholders and switches boot entry to entry-client.tsx. |
| frontend/Dockerfile | Replaces Dockerfile content (currently invalid) and appears intended to defer to Containerfile. |
| frontend/Containerfile | Adds multi-stage container build for dev/build/production. |
Files not reviewed (1)
- frontend/package-lock.json: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…in full pod
frontend/Dockerfile was committed as a regular file containing the literal
text "Containerfile" (a symlink that landed as a plain file, likely from a
core.symlinks=false checkout). BuildKit parsed it as an instruction, failing
the full-pod integration build with:
dockerfile parse error on line 1: unknown instruction: Containerfile
- Restore frontend/Dockerfile as a real symlink -> Containerfile (the intent
of commit "rename to Containerfile and symlink Dockerfile").
- Pin `dockerfile: Containerfile` on the frontend service in
integration/pods/full/docker-compose.yaml so CI is unaffected even if the
symlink is re-mangled by a core.symlinks=false environment.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…on RUN
The full-pod frontend build reached Containerfile:18 and failed with exit
code 2:
RUN set -euxo pipefail && groupadd ... && useradd ...
node:22-bookworm-slim runs RUN steps under /bin/sh (dash), which does not
support `set -o pipefail` ("Illegal option -o pipefail", exit 2). The command
has no pipeline, so pipefail is unnecessary; use `set -eux` instead.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c14efca to
37a3274
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- frontend/package-lock.json: Generated file
Comments suppressed due to low confidence (2)
frontend/server.js:184
- The injected modulepreload link hard-codes an absolute
/assets/...URL. IfBASEis set to serve the app under a non-root prefix (and static files are mounted atbasePath), this preload URL will 404. UsebasePathwhen constructing the preload URL.
const finchChunk = assetFiles.find((f) => /^finch\.es-.*\.js$/.test(f));
if (finchChunk) {
const preload = `<link rel="modulepreload" crossorigin href="/assets/${finchChunk}">`;
templateHtml = templateHtml.replace('</head>', ` ${preload}\n </head>`);
}
frontend/src/entry-client.tsx:29
- If
#auth-stateexists but has emptytextContent, the function returns early and leaves the element in the DOM (thefinally { element.remove() }never runs). Remove the element whenever it exists so the auth state isn’t left hanging around in the page.
function readInitialAuthState(): AuthState {
const element = document.getElementById('auth-state')
if (!element?.textContent) {
return { status: 'authFailed', authenticated: false }
}
try {
return JSON.parse(element.textContent) as AuthState
} catch {
return { status: 'authFailed', authenticated: false }
} finally {
element.remove()
}
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- frontend/package-lock.json: Generated file
Suppressed comments (5)
frontend/server.js:242
- The Express proxy rewrite for config differs from the Vite dev proxy config (
vite.config.ts). Currently/api/config/...is forwarded as/api/v1/api/config/...instead of rewriting the prefix to/api/v1/..., so dev behavior (standalone Vite) and prod/middleware behavior will diverge.
app.use('/api/config', createProxyMiddleware({
target: CONFIG_TARGET, changeOrigin: true,
pathRewrite: (path) => '/api/v1' + path,
}));
frontend/server.js:246
- The Express proxy rewrite for control differs from the Vite dev proxy config (
vite.config.ts). Currently/api/control/...is forwarded as/api/v1/api/control/...instead of rewriting the prefix to/api/v1/..., so dev behavior (standalone Vite) and prod/middleware behavior will diverge.
app.use('/api/control', createProxyMiddleware({
target: CONTROL_TARGET, changeOrigin: true,
pathRewrite: (path) => '/api/v1' + path,
}));
frontend/server.js:238
- The Express proxy rewrite for presets differs from the Vite dev proxy config (
vite.config.ts) and the documented client contract (src/api/presets.ts). With the current logic, a request like/api/presets/scan-presetsis forwarded as/api/v1/api/presets/scan-presets, which is not equivalent to the Vite rewrite to/api/v1/scan-presetsand will behave differently between dev and production/middleware mode.
app.use('/api/presets', requireAdminWrite, createProxyMiddleware({
target: PRESETS_TARGET, changeOrigin: true,
pathRewrite: (path) => '/api/v1' + path,
}));
frontend/server.js:101
isAdminPathcheckspathname.startsWith(basePath)without a path-segment boundary. IfBASEis set (e.g./app) then a request to/app2/adminwould incorrectly be treated as under the app base and could cause incorrect auth decisions (401/403) and routing behavior.
const pathname = new URL(url, RELATIVE_URL_PARSE_BASE).pathname;
const appPath = pathname.startsWith(basePath)
? `/${pathname.slice(basePath.length).replace(/^\/+/, '')}`
: pathname;
frontend/package.json:22
http-proxy-middleware@^4.1.1declares a Node engine requirement of^22.15.0 || ^24.0.0 || >=26.0.0(per package-lock). Without an explicitengines.nodein this package, local dev/CI can accidentally run with an older Node version and fail at install/runtime.
"express": "^5.2.1",
"express-correlation-id": "^3.0.1",
"http-proxy-middleware": "^4.1.1",
"react": "^18.2.0",
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- frontend/package-lock.json: Generated file
Suppressed comments (2)
frontend/server.js:186
- The injected Finch modulepreload link hard-codes an absolute "/assets/..." URL, which ignores the configured
basePath. If the app is deployed under a non-root base (e.g.BASE=/ios), the preload will point to the wrong location and fail to warm the Finch chunk.
const preload = `<link rel="modulepreload" crossorigin href="/assets/${finchChunk}">`;
frontend/src/components/ClientFinchBridge.tsx:23
useState<any>drops type safety even thoughloadFinch()is already typed. Using a typed module shape avoids accidental misuse ofFinchModuleand keeps this bridge refactor-safe.
const [FinchModule, setFinchModule] = useState<any>(null);
|
Padraic Shafer (@padraic-shafer) i think i got the essense of what you were suggesting - |
Padraic Shafer (padraic-shafer)
left a comment
There was a problem hiding this comment.
These updates are great! Thank you for making these changes to use auth scopes, admin paths, role conversions, and other simplifications.
Padraic Shafer (padraic-shafer)
left a comment
There was a problem hiding this comment.
The changes in commit
de8f55a look good.
|
Padraic Shafer (@padraic-shafer) if everything looks good, can we merge these changes ? |
Padraic Shafer (padraic-shafer)
left a comment
There was a problem hiding this comment.
Thank you for making these updates.
The new state of Auth looks significantly more flexible and maintainable.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- frontend/package-lock.json: Generated file
Suppressed comments (6)
frontend/Dockerfile:1
- This file is no longer a valid Dockerfile and will break any tooling that still builds using the default
Dockerfilepath (e.g.,docker build .without-f). If the intent is a rename, consider deletingDockerfileentirely, or keep it as a real Dockerfile (duplicate content), or make it a symlink toContainerfile(if your repo/platform supports symlinks).
FROM node:22-bookworm-slim AS base
frontend/server.js:187
- The injected preload URL is hardcoded to
/assets/...and ignoresbasePath. In production you mount static files atapp.use(basePath, sirv(...)), so for non-root bases the correct href should be under${basePath}/assets/...to avoid broken modulepreload links.
const preload = `<link rel="modulepreload" crossorigin href="/assets/${finchChunk}">`;
templateHtml = templateHtml.replace('</head>', ` ${preload}\n </head>`);
frontend/package.json:19
- There’s a version mismatch risk here: runtime
expressis v5 while@types/expressis v4, and@types/nodeis pinned to a much newer major than the Node 22 runtime used in the container image. This can lead to incorrect/unsupported API typings and build-time confusion; align@types/*majors with the actual runtime majors (or remove@types/expressif you’re not typechecking Express code).
"express": "^5.2.1",
frontend/package.json:29
- There’s a version mismatch risk here: runtime
expressis v5 while@types/expressis v4, and@types/nodeis pinned to a much newer major than the Node 22 runtime used in the container image. This can lead to incorrect/unsupported API typings and build-time confusion; align@types/*majors with the actual runtime majors (or remove@types/expressif you’re not typechecking Express code).
"@types/express": "^4.17.21",
"@types/node": "^26.0.1",
frontend/src/components/ClientFinchBridge.tsx:23
- Using
anyhere drops type safety forFinchConfigProvider/HubAppLayoutand route/config props. Consider typing this state as the Finch module type (e.g.,typeof import('@blueskyproject/finch') | null) so consumers get correct prop/type checking.
const [FinchModule, setFinchModule] = useState<any>(null);
frontend/vite.config.ts:10
- If these are expected to come from a local
.envfile, Vite won’t populate arbitrary keys intoprocess.envunless the config explicitly loads them (e.g., vialoadEnv). Either load env values in the Vite config or document that these must be provided by the OS/container environment; otherwise proxy targets may silently fall back to localhost defaults.
const PRESETS_TARGET = process.env.PRESETS_TARGET || 'http://localhost:8005'
const CONFIG_TARGET = process.env.CONFIG_TARGET || 'http://localhost:8004'
const CONTROL_TARGET = process.env.CONTROL_TARGET || 'http://localhost:8003'
| app.use('/api/presets', requireAdminWrite, createProxyMiddleware({ | ||
| target: PRESETS_TARGET, changeOrigin: true, | ||
| pathRewrite: (path) => '/api/v1' + path, | ||
| })); | ||
| app.use('/api/config', createProxyMiddleware({ | ||
| target: CONFIG_TARGET, changeOrigin: true, | ||
| pathRewrite: (path) => '/api/v1' + path, | ||
| })); | ||
| app.use('/api/control', createProxyMiddleware({ | ||
| target: CONTROL_TARGET, changeOrigin: true, | ||
| pathRewrite: (path) => '/api/v1' + path, | ||
| })); |
This pull request makes significant improvements to the frontend architecture, focusing on secure environment configuration, robust authentication and authorization, server-side rendering (SSR) support, and a more flexible build and deployment pipeline. The changes introduce a custom Express-based server for both development and production, SSR integration, and a new authentication context that controls route access based on user roles.
The most important changes are:
Server and Build System Overhaul
Dockerfilewith a new multi-stageContainerfilethat supports non-root builds, development, production, and SSR, and switches the project to use a customserver.jsfor both environments. (frontend/Containerfile[1]frontend/Dockerfile[2] [3].env.examplewith clear documentation for all runtime and build-time environment variables, including SSL, backend targets, and Vite client settings. (frontend/.env.examplefrontend/.env.exampleR1-R23)Custom Express Server with SSR and Proxy
server.jsthat serves both SSR and static assets, proxies API requests to backend services, enforces authentication and admin authorization, and supports HTTPS if configured. (frontend/server.jsfrontend/server.jsR1-R323)frontend/server.jsfrontend/server.jsR1-R323)Authentication and Authorization Framework
AuthContextand provider with utilities for checking authentication and scopes, making user roles and permissions available throughout the app. (frontend/src/contexts/AuthContext.tsxfrontend/src/contexts/AuthContext.tsxR1-R46)frontend/server.js[1]frontend/src/App.tsx[2] [3]SSR Compatibility and Finch Integration
ClientFinchBridgecomponent that loads the Finch UI only on the client, preventing SSR crashes due to Finch’s browser-only code. (frontend/src/components/ClientFinchBridge.tsx[1]frontend/src/components/finchLoader.ts[2]frontend/index.htmlfrontend/index.htmlL7-R13)Dependency and Script Updates
package.jsonscripts to support the new SSR build process and adds necessary dependencies for Express, proxying, SSR, and compression. (frontend/package.jsonfrontend/package.jsonL7-R29)These changes lay the groundwork for a secure, SSR-capable frontend with robust role-based access control and a flexible, production-ready deployment pipeline.