Implement server-side rendering (SSR) #67
Conversation
…including routing setup and environment checks for browser compatibility.
There was a problem hiding this comment.
Pull request overview
This pull request implements server-side rendering (SSR) for the application, transforming it from a client-only Single Page Application to a universal app with both server and client rendering capabilities. The changes migrate from Vite's dev server with nginx-based deployment to an Express-based SSR server using React Router v7's SSR APIs.
Changes:
- Migrated from client-only rendering to SSR with separate client and server entry points
- Replaced Vite's built-in proxy with Express middleware-based API proxying
- Added SSR safety checks throughout components to handle browser-only APIs (localStorage, document, window)
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Configured dual build outputs for client and server bundles with SSR-specific options |
| tsconfig.node.json | Extended configuration to include new server TypeScript files |
| package.json | Updated scripts for SSR builds and added Express, proxy middleware, and tsx dependencies |
| server.ts | New development SSR server with Vite middleware and API proxying |
| server.prod.ts | New production SSR server for serving pre-built client/server bundles |
| src/entry-server.tsx | Server-side render function using React Router v7 static rendering APIs |
| src/entry-client.tsx | Client-side hydration entry point |
| src/routes.tsx | Centralized route definitions with RootLayout wrapper |
| src/lib/history.ts | Added browser environment checks for localStorage access |
| src/components/* | Added SSR safety checks for document/window access across multiple components |
| index.html | Added SSR outlet placeholder for injecting server-rendered HTML |
| Dockerfile | Converted to Node.js-based production image with SSR server |
| docker-compose.dev.yml | Updated to run Express SSR dev server instead of nginx |
| src/main.tsx, src/App.tsx, src/App.css | Removed files replaced by new SSR architecture |
| nginx.dev.conf | Removed - API proxying now handled by Express middleware |
Comments suppressed due to low confidence (1)
nginx.dev.conf:1
- The
proxy_ssl_verify off;directive in this Nginx config disables TLS certificate verification for requests tohttps://dev1.player.eosc-data-commons.eu/. With certificate checks turned off, any network attacker between this Nginx instance and the upstream host can impersonate the player API, intercept or alter responses, and potentially exfiltrate sensitive data. Remove this directive or restrict it to a tightly controlled non-production environment where TLS certificates are still properly validated for any sensitive traffic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 26 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| app.use(async (req, res, next) => { | ||
| const url = req.originalUrl; | ||
|
|
||
| try { | ||
| // Read index.html | ||
| const template = await vite.transformIndexHtml( | ||
| url, | ||
| fs.readFileSync( | ||
| path.resolve(__dirname, 'index.html'), | ||
| 'utf-8' | ||
| ) | ||
| ); | ||
|
|
||
| // Load the server entry | ||
| const {render} = await vite.ssrLoadModule('/src/entry-server.tsx'); | ||
|
|
||
| // Render the app | ||
| const result = await render(req); | ||
|
|
||
| // Handle redirects | ||
| if (result.redirect) { | ||
| return res.redirect(result.status || 302, result.redirect); | ||
| } | ||
|
|
||
| // Inject the app-rendered HTML into the template | ||
| const html = template.replace('<!--ssr-outlet-->', result.html); | ||
|
|
||
| res.status(200).set({'Content-Type': 'text/html'}).end(html); | ||
| } catch (e) { | ||
| // If an error is caught, let Vite fix the stack trace | ||
| vite.ssrFixStacktrace(e as Error); | ||
| next(e); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The server.ts file handles all routes with the SSR middleware, including API routes. However, the API proxy middlewares are registered before the SSR handler, which means API requests will be proxied correctly. That said, the SSR handler should only handle HTML pages, not static assets. Consider adding a check to skip SSR for asset requests (e.g., requests with file extensions like .js, .css, .png, etc.) to avoid unnecessary processing and improve performance.
| # Start the SSR server | ||
| CMD ["npx", "tsx", "server.prod.ts"] |
There was a problem hiding this comment.
The Dockerfile uses npx tsx to run the production server, which requires the tsx package to be available. However, tsx is listed in dependencies (line 32 of package.json), which is correct. But running TypeScript files directly in production using tsx is not optimal - it adds runtime overhead for transpilation. Consider compiling server.prod.ts to JavaScript during the build step and running the compiled JavaScript file in production for better performance and reliability.
| "class-variance-authority": "^0.7.1", | ||
| "express": "^5.2.1", | ||
| "http-proxy-middleware": "^3.0.5", | ||
| "isbot": "^5.1.32", |
There was a problem hiding this comment.
The isbot package is listed as a dependency but is not used anywhere in the codebase. This package is typically used to detect bot requests in SSR applications to avoid unnecessary rendering or tracking. Consider either removing this unused dependency or implementing bot detection to optimize SSR performance by potentially skipping expensive operations for bot requests.
| useEffect(() => { | ||
|
|
There was a problem hiding this comment.
The empty lines added in these useEffect hooks appear to be unintentional formatting changes. These blank lines don't add value and are inconsistent with the rest of the codebase. Consider removing them to maintain consistent code style.
| useEffect(() => { | ||
|
|
There was a problem hiding this comment.
An empty line was added at the start of this useEffect hook. This appears to be an unintentional formatting change that doesn't add value and is inconsistent with the rest of the codebase. Consider removing it to maintain consistent code style.
| const handleDownload = () => { | ||
|
|
There was a problem hiding this comment.
An empty line was added at the start of this handleDownload function. This appears to be an unintentional formatting change that doesn't add value and is inconsistent with the rest of the codebase. Consider removing it to maintain consistent code style.
| app.use(async (req, res, next) => { | ||
| try { | ||
| const result = await render(req); | ||
|
|
||
| // Handle redirects | ||
| if (result.redirect) { | ||
| return res.redirect(result.status || 302, result.redirect); | ||
| } | ||
|
|
||
| // Inject the app-rendered HTML into the template | ||
| const html = template.replace('<!--ssr-outlet-->', result.html); | ||
|
|
||
| res.status(200).set({'Content-Type': 'text/html'}).end(html); | ||
| } catch (e) { | ||
| console.error('SSR Error:', e); | ||
| next(e); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Similarly, the production server should only invoke SSR for HTML page requests, not for static assets. While express.static will serve assets correctly, the middleware still runs the SSR handler for all unmatched routes. Consider adding a check to skip SSR for asset requests (e.g., requests with file extensions) to avoid unnecessary processing and improve performance.
No description provided.