feat: enterprise-grade middleware stack and frontend resilience#37
Conversation
…ssion, CORS, request logging, graceful shutdown, AppError, structured logging - Add helmet for security headers (X-Content-Type-Options, X-Frame-Options, XSS, etc.) - Add compression middleware for gzip/deflate response compression - Add CORS middleware for REST API with configurable origins - Add X-Request-ID generation/propagation for request correlation - Add pino-http structured request logging (auto-mutes health checks) - Add graceful shutdown (SIGTERM/SIGINT) with connection draining - Add AppError class with status codes, error codes, factory methods - Add request body size limits (1MB) to prevent payload abuse - Replace all console.log/error with structured pino logger - Improve global error handler with requestId, structured logging, AppError support - Update .env.example with new configuration variables - All 178 tests pass Agent-Logs-Url: https://github.com/OlympusLedgerOrg/TiM/sessions/c029bac5-29b7-427e-89f8-70aff7750a57 Co-authored-by: OlympusLedgerOrg <240737312+OlympusLedgerOrg@users.noreply.github.com>
…I client, env config, fix build - Add ErrorBoundary component with recovery UI (try again + reload) - Add React.lazy + Suspense for route-level code splitting - Add centralized apiClient with auth injection, timeout, structured errors - Add environment config utility (config/env.ts) - Fix vite.config.ts manualChunks to use function-based chunking (resolves UI5 build error) - Fix PWA injectManifest maximumFileSizeToCacheInBytes for large UI5 bundle - Add path alias (@/) support in vite.config.ts - Frontend builds successfully, backend 178 tests pass Agent-Logs-Url: https://github.com/OlympusLedgerOrg/TiM/sessions/c029bac5-29b7-427e-89f8-70aff7750a57 Co-authored-by: OlympusLedgerOrg <240737312+OlympusLedgerOrg@users.noreply.github.com>
…mit, CSP docs, consistent patterns Agent-Logs-Url: https://github.com/OlympusLedgerOrg/TiM/sessions/c029bac5-29b7-427e-89f8-70aff7750a57 Co-authored-by: OlympusLedgerOrg <240737312+OlympusLedgerOrg@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR upgrades TiM’s backend middleware stack (security, observability, resilience) and improves frontend resilience/performance via route-level code splitting, an app-wide error boundary, and new configuration/service utilities.
Changes:
- Backend: adds request ID propagation, structured request logging (pino-http), CORS + compression + helmet integration, graceful shutdown, and a structured
AppErrortype with enhanced global error handling. - Frontend: adds React.lazy/Suspense route splitting, an
ErrorBoundary, Vite alias + updated chunking rules, and introducesapiClient+envconfig helpers. - Tooling/config: updates backend dependencies and
.env.example; adjusts auth test expectations for the new error response shape.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/vite.config.ts | Adds @ alias, increases PWA cache limit, switches manualChunks to function-based splitting. |
| frontend/src/services/apiClient.ts | Introduces centralized fetch wrapper with timeout + typed ApiError. |
| frontend/src/main.tsx | Converts routes to lazy-loaded components; wraps router in ErrorBoundary + Suspense. |
| frontend/src/config/env.ts | Adds centralized Vite env access helper. |
| frontend/src/components/ErrorBoundary.tsx | Adds global React error boundary with recovery UI. |
| backend/tests/auth.test.ts | Updates assertions for new error response shape (message + optional requestId). |
| backend/src/services/movementService.ts | Replaces console.error with structured logger. |
| backend/src/services/labReportService.ts | Replaces console.error with structured logger. |
| backend/src/middleware/requestLogger.ts | Adds pino-http middleware with requestId integration and health-check noise suppression. |
| backend/src/middleware/requestId.ts | Adds request ID propagation/generation and response header echo. |
| backend/src/middleware/cors.ts | Adds REST API CORS policy with env-based allowlist and requestId exposure. |
| backend/src/lifecycle/shutdown.ts | Adds SIGTERM/SIGINT graceful shutdown orchestration (Socket.IO, HTTP, Prisma). |
| backend/src/errors/AppError.ts | Adds typed application error with status/code factories for consistent error envelopes. |
| backend/src/app.ts | Wires enterprise middleware stack, structured error handler, and graceful shutdown registration. |
| backend/package.json | Adds dependencies for helmet/cors/compression and pino-http + type packages. |
| backend/package-lock.json | Locks new backend dependencies. |
| backend/.env.example | Documents new env vars (CORS, rate limits, HTTPS redirect toggle, body limits). |
Files not reviewed (1)
- backend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Enterprise error handler also includes a requestId for traceability | ||
| if (res.body.requestId) { | ||
| expect(typeof res.body.requestId).toBe('string'); | ||
| } |
There was a problem hiding this comment.
The test now only asserts on requestId if it exists, which can mask regressions (the new middleware stack is supposed to always attach a request ID). Consider asserting that res.body.requestId is always present and a string (and optionally matches a UUID format) to lock in the new contract.
| // Enterprise error handler also includes a requestId for traceability | |
| if (res.body.requestId) { | |
| expect(typeof res.body.requestId).toBe('string'); | |
| } | |
| // Enterprise error handler always includes a requestId for traceability | |
| expect(res.body.requestId).toBeDefined(); | |
| expect(typeof res.body.requestId).toBe('string'); |
| ui5: ['@ui5/webcomponents', '@ui5/webcomponents-fiori', '@ui5/webcomponents-react'], | ||
| charts: ['recharts'], | ||
| manualChunks(id) { | ||
| if (id.includes('node_modules/react-dom/') || id.includes('node_modules/react/') || id.includes('node_modules/react-router/')) { |
There was a problem hiding this comment.
manualChunks vendor condition checks node_modules/react-router/, but the dependency in this repo is react-router-dom (see package-lock). This likely leaves react-router-dom out of the vendor chunk, reducing the effectiveness/predictability of the chunk split. Consider matching node_modules/react-router-dom/ (and optionally node_modules/react-router/) in the vendor branch.
| if (id.includes('node_modules/react-dom/') || id.includes('node_modules/react/') || id.includes('node_modules/react-router/')) { | |
| if ( | |
| id.includes('node_modules/react-dom/') || | |
| id.includes('node_modules/react/') || | |
| id.includes('node_modules/react-router-dom/') || | |
| id.includes('node_modules/react-router/') | |
| ) { |
| const API_BASE = '/api/v1'; | ||
| const DEFAULT_TIMEOUT_MS = 30_000; |
There was a problem hiding this comment.
The file header comment says the base URL is resolved from environment, but API_BASE is currently hard-coded to '/api/v1'. Since src/config/env.ts introduces VITE_API_BASE, consider sourcing the base URL from there (or updating the comment) so deploys can target different API hosts/prefixes without code changes.
|
|
||
| try { | ||
| const res = await fetch(url, { | ||
| ...init, | ||
| signal: controller.signal, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| ...(skipAuth ? {} : getAuthHeaders()), | ||
| ...(init.headers as Record<string, string> | undefined), | ||
| }, |
There was a problem hiding this comment.
init.headers can be a Headers instance or [string, string][], but this code spreads it as a Record<string,string>. At runtime spreading a Headers object results in no enumerable keys, so caller-provided headers may be silently dropped. Consider normalizing with new Headers(init.headers) and then setting defaults onto that, or accepting only object headers in the API type.
| try { | |
| const res = await fetch(url, { | |
| ...init, | |
| signal: controller.signal, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| ...(skipAuth ? {} : getAuthHeaders()), | |
| ...(init.headers as Record<string, string> | undefined), | |
| }, | |
| const headers = new Headers(init.headers); | |
| if (!headers.has('Content-Type')) { | |
| headers.set('Content-Type', 'application/json'); | |
| } | |
| if (!skipAuth) { | |
| for (const [key, value] of Object.entries(getAuthHeaders())) { | |
| if (!headers.has(key)) { | |
| headers.set(key, value); | |
| } | |
| } | |
| } | |
| try { | |
| const res = await fetch(url, { | |
| ...init, | |
| signal: controller.signal, | |
| headers, |
| <div style={{ | ||
| display: 'flex', | ||
| justifyContent: 'center', | ||
| alignItems: 'center', | ||
| minHeight: '60vh', | ||
| fontFamily: "'72', Arial, sans-serif", | ||
| color: '#6a6d70', | ||
| }}> | ||
| <div style={{ textAlign: 'center' }}> | ||
| <div style={{ fontSize: '2rem', marginBottom: '0.5rem' }}>⏳</div> |
There was a problem hiding this comment.
RouteLoadingFallback renders a purely visual loading indicator without any accessibility semantics. Consider adding role="status"/aria-live="polite" (and/or aria-busy) to the container so screen readers announce route transitions, and mark decorative glyphs as aria-hidden if you keep them.
| <div style={{ | |
| display: 'flex', | |
| justifyContent: 'center', | |
| alignItems: 'center', | |
| minHeight: '60vh', | |
| fontFamily: "'72', Arial, sans-serif", | |
| color: '#6a6d70', | |
| }}> | |
| <div style={{ textAlign: 'center' }}> | |
| <div style={{ fontSize: '2rem', marginBottom: '0.5rem' }}>⏳</div> | |
| <div | |
| role="status" | |
| aria-live="polite" | |
| aria-busy="true" | |
| style={{ | |
| display: 'flex', | |
| justifyContent: 'center', | |
| alignItems: 'center', | |
| minHeight: '60vh', | |
| fontFamily: "'72', Arial, sans-serif", | |
| color: '#6a6d70', | |
| }} | |
| > | |
| <div style={{ textAlign: 'center' }}> | |
| <div aria-hidden="true" style={{ fontSize: '2rem', marginBottom: '0.5rem' }}>⏳</div> |
| <p style={{ margin: '0 0 1.5rem', color: '#6a6d70', fontSize: '0.875rem', lineHeight: 1.5 }}> | ||
| An unexpected error occurred. Your work has been preserved. | ||
| {this.state.error && ( | ||
| <span style={{ display: 'block', marginTop: '0.75rem', fontFamily: 'monospace', fontSize: '0.8rem', color: '#bb0000' }}> | ||
| {this.state.error.message} | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
The fallback UI renders this.state.error.message directly to end users. In production this can leak internal details (including potentially sensitive data) from thrown errors. Consider only showing the raw message in development (or behind a debug flag) and keep the production message generic while logging the full error for diagnostics.
| return callback(null, true); | ||
| } | ||
|
|
||
| callback(new Error(`Origin ${origin} not allowed by CORS`)); |
There was a problem hiding this comment.
When an origin is not allowed, the middleware calls callback(new Error(...)). That error has no HTTP status, so the global error handler will treat it as a 500 and return "Internal server error" rather than a clear 403/401-style response. Consider throwing an AppError.forbidden(...) (or setting err.status = 403) so disallowed origins are handled as a client error instead of a server fault.
| callback(new Error(`Origin ${origin} not allowed by CORS`)); | |
| const err = new Error(`Origin ${origin} not allowed by CORS`) as Error & { | |
| status?: number; | |
| statusCode?: number; | |
| }; | |
| err.status = 403; | |
| err.statusCode = 403; | |
| return callback(err); |
Summary
Elevates TiM from MVP to enterprise-grade with comprehensive backend hardening and frontend resilience improvements. What SAP wishes they could be — but actually done right.
Backend Changes
Security Hardening
CORS_ORIGINenv var and exposedX-Request-IDheaderREQUEST_BODY_LIMITenv var (default 1MB) to prevent payload abuseObservability
console.log/console.errorreplaced with structured pino loggerResilience
compressionmiddlewareAppError.notFound(),AppError.conflict(), etc.)requestIdin all error responses for client-side debuggingConfiguration
.env.exampleupdated with all new environment variables:CORS_ORIGIN,REQUEST_BODY_LIMIT,RATE_LIMIT_*,DISABLE_HTTPS_REDIRECTFrontend Changes
Crash Resilience
Performance
@ui5/webcomponentsbuild resolution error; properly splits vendor/ui5/charts bundlesmaximumFileSizeToCacheInBytesraised to 15MB to accommodate UI5 bundleDeveloper Experience
apiClient) — Automatic Bearer token injection, 30s timeout, structuredApiErrorclass, typed GET/POST/PUT/PATCH/DELETE methodsconfig/env.ts) — Centralized Vite env access with proper TypeScript interface@/) — Configured in vite.config.tsValidation
npm run buildsucceedsnpm run buildsucceeds (main app + PWA service worker)